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 dependencies to support illumos target #8093

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

pfmooney
Copy link
Contributor

Several dependencies of cargo require updates in order to build with illumos as the target_os platform (rust-lang/rust#55553). Most are patch revisions to include #[cfg()] updates. In the case of fs2, the maintainer does not appear to be minding activity on the project, so it was forked into fs3, maintaining the same interfaces and logic (but featuring the widened platform support).

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2020
Cargo.toml Outdated
flate2 = { version = "1.0.3", default-features = false, features = ["zlib"] }
fs2 = "0.4"
git2 = "0.13.0"
fs3 = "0.5"
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit hesitant to bring in a whole new dependency for this, if the previous crate is unmaintained can the supporting code be copied into Cargo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate the desire to minimize the change here. Really fs3 is merely a fork of `fs2 with a few changes made:

  1. Widen #[cfg()] guards to support illumos
  2. Restructure tests so they can be run under stable as well as nightly (split out libtest-dependent bits)
  3. Reconfigure CI so test suite runs again (successfully, of course)
  4. Update to Rust 2018 (mainly try! to ? conversions)

The intent and hope is to keep it in good working order.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense that changes were necessary! I think though if the original author can't be reached I'd still personally prefer to inline the code necessary since we don't use all that much of the crate.

@pfmooney
Copy link
Contributor Author

Refactored to inline the flock()-related code from fs2


/// Simulate flock() using fcntl(); primarily for Oracle Solaris.
#[cfg(target_os = "solaris")]
fn flock(file: &File, flag: libc::c_int) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

I've got a few thoughts about this implementation. First off it seems like it's different enough from flock that it may want its own little submodule too?

Additionally I was reviewing the differences between fcntl vs flock-based locks. I don't think fcntl is what we want in terms of synchronization in that it releases the lock if any file description in the process closes the file or it also doesn't protect against threads. I don't think that Cargo runs afoul of this today but I don't think we can really guarantee that we won't ever run afoul of this ever.

If solaris doesn't implement the flock-style file locking, could solaris perhaps just eschew locking entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the fcntl-style locking is an ugly hack. In illumos, we ended up adding a "real" flock implementation to the OS. Considering the existing bail-outs for NFS and other filesystems which don't tolerate locking, it's probably OK to use no-op locking for solaris targets. I suspect that illumos systems awaiting their own target_os are probably the biggest pool of systems using the solaris target today.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah in general file locking is largely best-effort within Cargo and it's only relevant for concurrent Cargo invocations which are somewhat rare. Internally Cargo doesn't rely on file locks at all, so having it be a noop should keep everything largely working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great. I'm happy to see that stuff go.

This inlines the flock() logic from danburkert/fs2, which in its current
state does not compile on illumos (or certain other cfg(unix) targets).
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 13, 2020

📌 Commit c04a87c has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2020
@bors
Copy link
Contributor

bors commented Apr 13, 2020

⌛ Testing commit c04a87c with merge 74e3a7d...

@bors
Copy link
Contributor

bors commented Apr 13, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 74e3a7d to master...

@bors bors merged commit 74e3a7d into rust-lang:master Apr 13, 2020
@pfmooney
Copy link
Contributor Author

Thanks for your help getting this in.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2020
Update cargo

12 commits in 390e8f245ef2cd7ac698b8a76abf029f9abcab0d..74e3a7d5b756d7c0e94399fc29fcd154e792c22a
2020-04-07 17:46:45 +0000 to 2020-04-13 20:41:52 +0000
- Update dependencies to support illumos target (rust-lang/cargo#8093)
- Whitelist another known spurious curl error (rust-lang/cargo#8102)
- Fix nightly test matching rustc "warning" output. (rust-lang/cargo#8098)
- Update default for codegen-units. (rust-lang/cargo#8096)
- Fix freshness when linking is interrupted. (rust-lang/cargo#8087)
- Add `cargo tree` command. (rust-lang/cargo#8062)
- Add "build-finished" JSON message. (rust-lang/cargo#8069)
- Extend -Zpackage-features with more capabilities. (rust-lang/cargo#8074)
- Disallow invalid dependency names through crate renaming (rust-lang/cargo#8090)
- Use the same filename hash for pre-release channels. (rust-lang/cargo#8073)
- Index the commands section (rust-lang/cargo#8081)
- Upgrade to mdBook v0.3.7 (rust-lang/cargo#8083)
@ehuss ehuss added this to the 1.44.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants