-
Notifications
You must be signed in to change notification settings - Fork 17
Tooling improvements #177
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
Tooling improvements #177
Conversation
Add -s option to do git tagging and gpg signing only on request, so it is easier to test the release script w/o messing with signatures. Ensure tarballs are actually compressed, change to tar.gz as that's the format git archive understand by default. Signed-off-by: Simo Sorce <[email protected]>
This way reviewers and committers have a checkilist to not forget about required steps Signed-off-by: Simo Sorce <[email protected]>
| tar --transform "s#^vendor#kryoptic-${VERSION}/vendor#" -czf kryoptic-vendor-${VERSION}.tar.xz vendor | ||
| tar --transform "s#^vendor#kryoptic-${VERSION}/vendor#" -czf kryoptic-vendor-${VERSION}.tar.gz vendor |
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 the vendor tarball should be named as %{name}-%{version}-vendor.tar.gz (that is at least what we have in RHEL's rust-sequoia-sq package now). I am thinking about adding also a rebuild in centos to test this code path and make sure the vendor tarball also works.
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.
at least that is what the instructions in the spec file say:
cargo download %%{crate}==%%{version} > %%{crate}-%%{version}.crate
tar xf %%{crate}-%%{version}.crate
cargo update ... # optional, if you want to update specific dependencies
pushd %%{crate}-%%{version}
cargo vendor-filterer --platform x86_64-unknown-linux-gnu \
--platform powerpc64le-unknown-linux-gnu \
--platform aarch64-unknown-linux-gnu \
--platform i686-unknown-linux-gnu \
--platform s390x-unknown-linux-gnu \
--all-features=true
Should we do the vendoring manually then or try to stick the the cargo tooling?
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.
Why do you need the transform here? When I tried to use this tarball, it creates the /src/rpmbuild/BUILD/kryoptic-1.0.0/kryoptic-1.0.0/vendor/ and the cargo does not find the vendored dependencies during the build (at least the way I wrote it now using rust-rpm macros).
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.
-
tarball names:
do we have any guideline anywhere? Otheriwise it looks like an arbitrary choice and I am ok with vendor before version, it is more natiral to have the version at the end. -
tooling:
I would use cargo tooling as much as possible to generate the right stuff, but I do not want to depend on crates.io which is why I generate the vendor directory, someone with the two tarballs (and the system dependencies necessary) should be able to make a hermetic build (no access to the netwrok). -
transform:
If you look into the tarball, if you do not do the transform you get "vendor" as the tarball root directory ... it sounds wrong to me, I think someone that just want to manually inspect the two tarballs and simply -xf them, should get everything under /kryoptic- and not one under that and the other under an unversioned /vendor. However if the tools can't deal with that packaging I can begrudgingly change that.
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.
Naming: I did not find the naming explained anywhere, but what rust2rpm generates is as follows "{project_name}-{project_version}-vendor.tar.xz":
https://pagure.io/fedora-rust/rust2rpm/blob/main/f/rust2rpm/vendor.py#_44
Tooling: We could use rust2rpm to generate the vendor tarball.
I think its not that the tools would not be able to handle that, but it will be more complicated. If we take the tarball as it is, the tooling will make it quite straightforward, see #178.
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.
It seems you have a better handle than I have with this aspects.
Do you want to take over this task in #178 and we close this PR ?
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.
All what I learned this afternoon while experimenting with CI rpmbuild ;)
Sure, we can take over this into the #178 if you are ok with that approach (I pulled your commits into that branch). I can see if we can get the vendoring simplified with rust2rpm tool.
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.
it seem those changes are required to get the RHEL rpm builds right, and while the lack of prefix in the vendor tarball irks me, it is not a big deal.
| done | ||
| shift "$(($OPTIND -1))" | ||
|
|
||
| if [ -z "$1"]; then |
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 does not work either
| if [ -z "$1"]; then | |
| if [ -z "$1" ]; then |
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 is just a style issue right ?
Because my testing shows that the script works correctly.
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.
it works, but throws error (which is ignored as the exit code of the condition does not make the failure of the whole script):
./misc/release.sh: line 19: [: missing `]'
| tar --transform "s#^vendor#kryoptic-${VERSION}/vendor#" -czf kryoptic-vendor-${VERSION}.tar.xz vendor | ||
| tar --transform "s#^vendor#kryoptic-${VERSION}/vendor#" -czf kryoptic-vendor-${VERSION}.tar.gz vendor |
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.
Why do you need the transform here? When I tried to use this tarball, it creates the /src/rpmbuild/BUILD/kryoptic-1.0.0/kryoptic-1.0.0/vendor/ and the cargo does not find the vendored dependencies during the build (at least the way I wrote it now using rust-rpm macros).
|
Obsoleted by #178 |
Improve non code aspects of the project