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.

Sign up for GitHub

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

Jump to bottom

Add a gh-action and buildomat jobs to cargo check on no-default-features and feature-powerset #6018

Merged
merged 4 commits into from
Jul 16, 2024
Merged

Add a gh-action and buildomat jobs to cargo check on no-default-features and feature-powerset #6018

merged 4 commits into from
Jul 16, 2024

Conversation

zeeshanlakhani
Copy link
Collaborator

@zeeshanlakhani zeeshanlakhani commented Jul 9, 2024 •
edited
Loading

Includes:

  • An xtask which
  • New CI jobs for checking-features in rust.yml & buildomat
  • Extends download xtask to install cargo-hack

@zeeshanlakhani zeeshanlakhani force-pushed the zl/gha-job-no-default-features branch 4 times, most recently from ec365cc to 7a2cc94 Compare July 9, 2024 04:49
@zeeshanlakhani zeeshanlakhani self-assigned this Jul 9, 2024
@sunshowers
Copy link
Contributor

Interesting -- was there something specific that broke?

@zeeshanlakhani
Copy link
Collaborator Author

zeeshanlakhani commented Jul 9, 2024 •
edited
Loading

Interesting -- was there something specific that broke?

well, it started with needing to test features around oxql/sql and having to run no-default-features manually :). But, I did find the issue w/ sqlformat on test(s), which was on main.

I'm about to push a feature-powerset addition to these, which caught some interesting jsonschema errors in uuid-kinds and oxql as well, so it would be good to get this in.

@sunshowers
Copy link
Contributor

Thanks. Curious to see what CI times would look like with --feature-powerset.

@zeeshanlakhani
Copy link
Collaborator Author

zeeshanlakhani commented Jul 9, 2024 •
edited
Loading

Thanks. Curious to see what CI times would look like with --feature-powerset.

yep, will time it on the next push. Local illumos machine runs are not bad, but worth seeing. Def. found some issues at least.

@zeeshanlakhani zeeshanlakhani marked this pull request as draft July 9, 2024 19:56
@zeeshanlakhani
Copy link
Collaborator Author

Will bring this out of draft after running the powerset. oxql/oximeter (which leveraged feature flags more than most) needed some re-arrangement too.

bnaecker
bnaecker previously approved these changes Jul 9, 2024
View reviewed changes
@zeeshanlakhani
Copy link
Collaborator Author

Thanks. Curious to see what CI times would look like with --feature-powerset.

local run:

real     3:01.539415366
user     1:51.732055422
sys      1:09.571580910

Pushing the updates now.

@zeeshanlakhani zeeshanlakhani force-pushed the zl/gha-job-no-default-features branch from 135c4c7 to 2ceba5e Compare July 9, 2024 21:42
@zeeshanlakhani zeeshanlakhani marked this pull request as ready for review July 9, 2024 21:42
@zeeshanlakhani zeeshanlakhani changed the title chore: no-default-features check due to feature-based compiles Add a gh-action and buildomat jobs to cargo check on no-default-features and feature-powerset Jul 9, 2024
@zeeshanlakhani zeeshanlakhani force-pushed the zl/gha-job-no-default-features branch from 2ceba5e to c7d5e59 Compare July 9, 2024 21:45
@zeeshanlakhani zeeshanlakhani marked this pull request as draft July 9, 2024 21:52
@zeeshanlakhani zeeshanlakhani force-pushed the zl/gha-job-no-default-features branch 4 times, most recently from ca175ec to 4e23c52 Compare July 9, 2024 22:39
@zeeshanlakhani zeeshanlakhani marked this pull request as ready for review July 9, 2024 22:47
@zeeshanlakhani
Copy link
Collaborator Author

zeeshanlakhani commented Jul 9, 2024 •
edited
Loading

@sunshowers 29 minutes on buildomat, 9 on gh-a (ubuntu); so not bad, especially compared to deploys :).

@zeeshanlakhani
Copy link
Collaborator Author

@bnaecker as mentioned, here's a take on the oxql feature-fixes. Down to bring in some of yours too, whichever you deem best.

@david-crespo
Copy link
Contributor

#5988 broke cargo build -p omicron-nexus for me locally and this PR fixes it.

image

@zeeshanlakhani zeeshanlakhani mentioned this pull request Jul 10, 2024
Merged
@zeeshanlakhani
Copy link
Collaborator Author

The xtask and ci stuff are still reviewable here. I'll rebase to accommodate the feature fixes in #6036 once that merges.

zeeshanlakhani added a commit that referenced this pull request Jul 10, 2024
@zeeshanlakhani
Fixes around feature-flagging, particularly in oxql, as well as uuid-…
808b1a1 
…kinds, sled-storage (#6036)

Related to #6018, but
without the CI/xtask bits.
@zeeshanlakhani
Copy link
Collaborator Author

rebased.

sunshowers
sunshowers reviewed Jul 11, 2024
View reviewed changes
.github/buildomat/jobs/check-features.sh Outdated Show resolved Hide resolved
.github/buildomat/jobs/check-features.sh Outdated Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
dev-tools/xtask/src/check_features.rs Outdated Show resolved Hide resolved
dev-tools/xtask/src/check_features.rs Outdated Show resolved Hide resolved
@zeeshanlakhani zeeshanlakhani force-pushed the zl/gha-job-no-default-features branch 6 times, most recently from 45be74a to a4f1097 Compare July 13, 2024 03:49
@zeeshanlakhani
Copy link
Collaborator Author

Pre-built updates added.

sunshowers
sunshowers reviewed Jul 15, 2024
View reviewed changes
Copy link
Contributor

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty great, just a few more questions.

dev-tools/xtask/src/check_features.rs Outdated Show resolved Hide resolved
dev-tools/xtask/src/check_features.rs Outdated Show resolved Hide resolved
tools/cargo_hack_checksum Show resolved Hide resolved
README.adoc Outdated Show resolved Hide resolved
@zeeshanlakhani
Copy link
Collaborator Author

zeeshanlakhani commented Jul 15, 2024 •
edited
Loading

Looks pretty great, just a few more questions.

@sunshowers Sounds good. Minus the CIDL origin, I pushed some changes out with a comment on the workspace/out dir functions.

sunshowers
sunshowers approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@zeeshanlakhani zeeshanlakhani enabled auto-merge (squash) July 15, 2024 22:26
@zeeshanlakhani zeeshanlakhani merged commit 86b1195 into main Jul 16, 2024
22 checks passed
@zeeshanlakhani zeeshanlakhani deleted the zl/gha-job-no-default-features branch July 16, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnaecker bnaecker bnaecker approved these changes

@sunshowers sunshowers sunshowers approved these changes

@iliana iliana Awaiting requested review from iliana

Assignees

@zeeshanlakhani zeeshanlakhani

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zeeshanlakhani @sunshowers @bnaecker @david-crespo