Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update oximeter and crucible deps #679

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Update oximeter and crucible deps #679

merged 1 commit into from
Apr 10, 2024

Conversation

bnaecker
Copy link
Contributor

@bnaecker bnaecker commented Apr 5, 2024

This pulls in omicron#5340, which lays some groundwork for larger changes to the oximeter producer-collector interface coming later.

This pulls in omicron#5340, which lays some groundwork for larger
changes to the oximeter producer-collector interface coming later.
@bnaecker bnaecker requested a review from pfmooney April 5, 2024 17:10
@leftwo
Copy link
Contributor

leftwo commented Apr 5, 2024

These are all the crucible changes included here:

Update oximeter dependency (#1244)
Dummy downstairs cleanup (#1247)
Some DTrace updates. (#1246)
HEY! LISTEN! HEY! LISTEN! (#1215)
Fix clippy lints (#1245)

We should rope in @jmpesp and be sure Omicron is ready to take the crucible 1215 PR.

@bnaecker bnaecker requested a review from jmpesp April 5, 2024 17:26
@bnaecker
Copy link
Contributor Author

bnaecker commented Apr 5, 2024

Thanks @leftwo. @jmpesp I've added you as a reviewer. I put this up to include a small change to the oximeter producer registration, in preparation for a bigger one coming later. Looks like this includes your changes in Crucible #1215, so can you check Omicron is ready for that to be included in its Propolis + Crucible revisions?

@jmpesp
Copy link
Contributor

jmpesp commented Apr 9, 2024

Sorry I missed these notifications - yes, Omicron is ready: oxidecomputer/omicron#5135 went in before 1215 did.

@bnaecker
Copy link
Contributor Author

bnaecker commented Apr 9, 2024

@jmpesp thanks for taking a look! Either you or @leftwo feel comfortable approving these? If not, we can wait for @pfmooney, there's not a lot of urgency IMO.

@askfongjojo
Copy link

Let's get this in (and bump the pinned version in omicron) since the upcoming customer release has a shorter runway this time.

@leftwo
Copy link
Contributor

leftwo commented Apr 10, 2024

I'm fine with the changes, and will hit merge, but for some reason I'm getting a bunch of warning when I compile this branch locally (on Atrium). I'm going to spend a little time and see if I can figure out why, but I suspect it's something local to my setup.

An example is:

    Checking cpuid_profile_config v0.0.0 (/home/alan/ws/propolis/crates/cpuid-profile-config)           
    Checking usdt v0.5.0                                      
   Compiling crucible-common v0.0.1 (https://github.com/oxidecomputer/crucible?rev=5677c7be81b60d9ba9c30991d10376f279a1d3b7#5
677c7be)                                                                                                                     
warning: the item `components` is imported redundantly        
 --> crates/propolis-api-types/src/instance_spec/v0/builder.rs:9:28                                                          
  |                                                                                                                          
9 | use crate::instance_spec::{components, v0::*, PciPath};                                                                  
  |                            ^^^^^^^^^^  ----- the item `components` is already imported here
  |                                                                                                                          
  = note: `#[warn(unused_imports)]` on by default                                                                            
                                                              
warning: the item `PciPath` is imported redundantly                                                                          
 --> crates/propolis-api-types/src/instance_spec/v0/builder.rs:9:47                                                          
  |                                                                                                                          
9 | use crate::instance_spec::{components, v0::*, PciPath};                                                                  
  |                                        -----  ^^^^^^^                                                                    
  |                                        |                                                                                 
  |                                        the item `PciPath` is already imported here    
                                                                                                                             
    Checking propolis-server-config v0.0.0 (/home/alan/ws/propolis/crates/propolis-server-config)                            
   Compiling num_enum_derive v0.7.0

@leftwo
Copy link
Contributor

leftwo commented Apr 10, 2024

This might explain my issues:

alan@atrium:propolis$ rustc --version
rustc 1.79.0-nightly (8b2459c1f 2024-04-09)

This works:

alan@atrium:propolis$ rustc --version
rustc 1.77.2 (25ef9e3d8 2024-04-09)

@leftwo leftwo merged commit 8ff3ab6 into master Apr 10, 2024
10 checks passed
@leftwo leftwo deleted the update-oximeter branch April 10, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants