-
Notifications
You must be signed in to change notification settings - Fork 198
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
build-sys: Switch to committing cxx.rs generated code #3864
Conversation
a5fbca7
to
e65c708
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes a lot of sense.
Having generated code in-tree is fine, but it then runs the risk of going out of sync. So IMO the convention should be that you also have CI that checks and fails if it wasn't regenerated.
Let's add a new GitHub Action workflow for that?
e65c708
to
9c6320e
Compare
OK did a lot more cleanups here, and also added the requested CI check! |
OK, for some reason the header ordering with |
9c6320e
to
e96a7f7
Compare
Did some searching and turned up Anyways, switching to piping with |
e96a7f7
to
1ddbbbf
Compare
set -xeuo pipefail | ||
CXX_VER=$(cargo metadata --format-version 1 | jq -r '.packages[]|select(.name == "cxx").version') | ||
mkdir -p target | ||
time cargo install --root=target/cxxbridge cxxbridge-cmd --version "${CXX_VER}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently duplicates ci/installdeps.sh
.
Thinking more on this, I think it makes sense to have ci/installdeps.sh
be a superset which includes installing cxx tooling, but maybe let's just have it execute this script like we do in ci/verify-cxx.sh
to avoid duplication? (Also the comment there needs an update.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I think of it, ci/installdeps.sh
is basically "stuff we need to build a source archive" (including vendor) to replicate more precisely the Koji-style offline build model.
So it shouldn't install cxx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, WFM!
Closes: coreos#3252 This avoids creating a magical, manual error prone step in the release process, at the cost of dealing with generated code in git.
1ddbbbf
to
28edfe6
Compare
set -xeuo pipefail | ||
CXX_VER=$(cargo metadata --format-version 1 | jq -r '.packages[]|select(.name == "cxx").version') | ||
mkdir -p target | ||
time cargo install --root=target/cxxbridge cxxbridge-cmd --version "${CXX_VER}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, WFM!
Closes: #3252
This avoids creating a magical, manual error prone step in the
release process, at the cost of dealing with generated code in git.