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

Automatically set BUILDHOST and tag and add COOKIE tag #118

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

dralley
Copy link
Collaborator

@dralley dralley commented Apr 8, 2023

Incremental improvement towards making an empty package built by rpm-r more similar to one created by rpmbuild

📜 Checklist

  • Commits are cleanly separated and have useful messages
  • A changelog entry or entries has been added to CHANGELOG.md
  • Documentation is thorough
  • Test coverage is excellent and passes
  • Works when tests are run --all-features enabled

@dralley dralley requested a review from drahnr April 8, 2023 18:25
@dralley dralley force-pushed the compatibility branch 3 times, most recently from 24a3494 to 2655586 Compare April 8, 2023 18:51
@dralley dralley changed the title Automatically set BUILDHOST and COOKIE tags Automatically set BUILDHOST and tag and add COOKIE tag Apr 8, 2023
@dralley dralley force-pushed the compatibility branch 2 times, most recently from 8e5227b to e713a98 Compare April 8, 2023 19:03
@dralley dralley requested a review from cmeister2 April 13, 2023 17:15
@@ -32,7 +34,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Breaking Changes

- Bump MSRV to 1.63.0
- Bump MSRV to 1.64.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be nice if we had an idea of what was pinning the MSRV high. It's better if the MSRV is lower, cause then implementors can have a lower MSRV and it's compatible with more versions of Rust.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case gethostname wanted an MSRV bump. With an older version of gethosname it could possibly be pushed lower.

pub fn get_build_cookie() -> String {
let build_time = SystemTime::now()
.duration_since(SystemTime::UNIX_EPOCH)
.unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an infallible operation? Might be nice to be expect() if so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It ought to be effectively infalliable since it only returns Err when "earlier is later than self". So the system timestamp would have to be less than 0.

@dralley
Copy link
Collaborator Author

dralley commented Apr 13, 2023

@cmeister2 Done

@dralley dralley removed the request for review from drahnr April 13, 2023 17:44
@dralley dralley merged commit b44b60b into rpm-rs:master Apr 13, 2023
@dralley dralley deleted the compatibility branch April 13, 2023 22:20
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

@cmeister2 @dralley

The changes in this PR are yepordizing binary reproducibility for sake of identical behavior to existing rpm tooling, which was never a goal for rpm-rs. I don't oppose the possibility of providing such data - adding timestamps and build host info should, at most, be an explicit OPT-IN as part of the builder, and not the default in any case.

@dralley
Copy link
Collaborator Author

dralley commented Apr 14, 2023

@drahnr See the 2nd-to-last commit on #115

And #117. We can discuss it there

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.

3 participants