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

Fix #4482 and #9449: set Fossil ignore and clean settings locally #9469

Merged
merged 1 commit into from
May 10, 2021
Merged

Fix #4482 and #9449: set Fossil ignore and clean settings locally #9469

merged 1 commit into from
May 10, 2021

Conversation

PaulDance
Copy link
Contributor

This aims to close #4482 and close #9449.

Context: currently, the Fossil extension for cargo new would call fossil settings [...] in order to configure the created repository to ignore and allow cleaning the target build directory. However, as #9449 shows, it is ran from the CWD, which is probably outside of the repo, therefore it modifies global settings instead of local ones. This PR fixes that by writing the settings to local versioned files as the issue recommends.

Furthermore, as #9449 notes, configuring the repository's ignore settings only in util::vcs::FossilRepo::init means it is not done when the repository is not new and makes it harder to maintain equivalent support for VCS ignore system implementations. It also completely ignores the --lib CLI option which adds Cargo.lock to the ignore list for Git and Mercurial.

Therefore, the following modifications have been performed, using the Fossil documentation as a reference for the ignore syntax:

  • All settings logic has been removed from FossilRepo::init.
  • ops::cargo_new::IgnoreList::push now requires a third argument for Fossil-specific glob syntax that is stored in a new fossil_ignore field.
  • IgnoreList::format_new uses the fossil_ignore field when needed just like any other VCS.
  • IgnoreList::format_existing handles Fossil separately for a few operations as its glob syntax does not support comments, so any lines starting with # cannot be included: the configurations can only be merged here.
  • write_ignore_file has been modified a bit as well to enable writing to two files for Fossil specifically in order to keep supporting its cleaning feature. The return type of the function is now CargoResult<()> instead CargoResult<String>: it makes the implementation easier and as the return value was actually not used, I figured it would be okay to do so, plus that return value would not make too much sense anymore for Fossil because of the two possibly different file contents.
  • mk has been updated to provide the correct ignore glob to IgnoreList::push for Fossil.

Previously, the Fossil extension for `cargo new` would call `fossil
settings [...]` in order to configure the created repository to ignore
and allow cleaning the `target` build directory. However, as #9449
shows, it is ran from the CWD, which is probably outside of the repo,
therefore it modifies global settings instead of local ones. This aims
to fix that by writing the settings to local versioned files as the
issue recommends.

Signed-off-by: Paul Mabileau <paulmabileau@hotmail.fr>
@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 @Eh2406 (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 May 9, 2021
@PaulDance
Copy link
Contributor Author

@Eh2406 The workflow seems to have failed on a weird APT error when installing build requisites. Would it be possible to restart the tests or should I fake a change in order to trigger that manually?

@ehuss
Copy link
Contributor

ehuss commented May 10, 2021

I just restarted them.

@PaulDance
Copy link
Contributor Author

PaulDance commented May 10, 2021

@ehuss Thanks! Oh, by the way, I noticed a small warning in the CI logs:

warning: use of deprecated associated function `url::Url::into_string`: use Into<String>
   --> src/registry.rs:183:26
    |
183 |                 dl_url().into_string(),
    |                          ^^^^^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

for cargo-test-support. It should simply be because it is not reported when building from the parent package. I thought something as small as this would not warrant a dedicated issue, so should I open a PR for this or do you think it would be faster for a team member to do so?

@ehuss
Copy link
Contributor

ehuss commented May 10, 2021

Thanks! The code is starting to get a little tangled, but I think it's fine for now. If more files or vcs systems are added in the future, we may want to consider abstracting and simplifying it somehow, though I'm not sure what that would look like.

@bors r+

@bors
Copy link
Contributor

bors commented May 10, 2021

📌 Commit 6801c49 has been approved by ehuss

@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 May 10, 2021
@bors
Copy link
Contributor

bors commented May 10, 2021

⌛ Testing commit 6801c49 with merge 5c45513...

@ehuss
Copy link
Contributor

ehuss commented May 10, 2021

should I open a PR

Please go ahead!

@PaulDance
Copy link
Contributor Author

Alright, cool, thank you very much!

@bors
Copy link
Contributor

bors commented May 10, 2021

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 5c45513 to master...

@bors bors merged commit 5c45513 into rust-lang:master May 10, 2021
@PaulDance PaulDance deleted the fossil-local-settings branch May 10, 2021 19:35
bors added a commit to rust-lang-ci/rust that referenced this pull request May 12, 2021
Update cargo

8 commits in e51522ab3db23b0d8f1de54eb1f0113924896331..070e459c2d8b79c5b2ac5218064e7603329c92ae
2021-05-07 21:29:52 +0000 to 2021-05-11 18:12:23 +0000
- Fix rustdoc warnings (rust-lang/cargo#9468)
- Improve performance of git status check in `cargo package`. (rust-lang/cargo#9478)
- Link to the new rustc tests chapter. (rust-lang/cargo#9477)
- Bump index cache version to deal with semver metadata version mismatch. (rust-lang/cargo#9476)
- Fix Url::into_string deprecation warning (rust-lang/cargo#9475)
- Fix rust-lang/cargo#4482 and rust-lang/cargo#9449: set Fossil ignore and clean settings locally (rust-lang/cargo#9469)
- Improve two error messages (rust-lang/cargo#9472)
- Fix `cargo install` with a semver metadata version. (rust-lang/cargo#9467)
@ehuss ehuss added this to the 1.54.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
5 participants