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 add-essential-files command #1966

Merged
merged 1 commit into from
Dec 25, 2022
Merged

Conversation

syphar
Copy link
Member

@syphar syphar commented Dec 20, 2022

when working on making the local build work on my ARM, I stumbled on this issue where running cratesfyi build add-essential-files won't work any more.

This could be a regression from #1892 , though I don't fully understand why the change was made in that PR.

Any insight? @jsha @jyn514

@syphar syphar changed the title fix add-essential-files shell command fix add-essential-files command Dec 20, 2022
@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Dec 20, 2022
@syphar syphar force-pushed the fix-essential-files branch from 5e5a3f4 to 93b8249 Compare December 20, 2022 14:34
@jyn514
Copy link
Member

jyn514 commented Dec 20, 2022

I suspect it was removed because it doesn't work for CI toolchains, only dist toolchains - @jsha can you confirm?

I think a better fix is to only run detect_toolchain_version for dist toolchains, instead of adding or removing it unconditionally.

@syphar
Copy link
Member Author

syphar commented Dec 20, 2022

I think a better fix is to only run detect_toolchain_version for dist toolchains, instead of adding or removing it unconditionally.

When I just used the result of detect_toolchain_version, and didn't set self.rustc_version, add_essential_files failed in a later step.

I'm happy to dig deeper when I know which way to go, @jsha do you have insight?

@jyn514
Copy link
Member

jyn514 commented Dec 25, 2022

I suspect it was removed because it doesn't work for CI toolchains, only dist toolchains - @jsha can you confirm?

Hmm, I added it back and it worked fine for CI toolchains. I think this is fine to merge as-is.

@jyn514 jyn514 merged commit ce8b117 into rust-lang:master Dec 25, 2022
@jyn514 jyn514 deleted the fix-essential-files branch December 25, 2022 17:25
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Dec 25, 2022
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Dec 25, 2022
@jsha
Copy link
Contributor

jsha commented Dec 26, 2022

I'm a bit late commenting on this, but in https://github.com/rust-lang/docs.rs/pull/1892/files#diff-b06b44e6583254ce69af8cedabc04a73a97599d593676e1bbad7ee442240ccf1R204-R205 I mentioned that detect_rustc_version errors if run with a CI toolchain. It's interesting to hear from @jyn514 that that's changed; or maybe I was incorrect when I made that comment.

The reason I removed the call to detect_rustc_version from add_essential_files was indeed because it would error if called on a CI toolchain. I worked around that by setting it in update_toolchain instead. My analysis at the time was that add_essential_files was always and only called from update_toolchain, which would have made this refactoring correct. But I see now that was incorrect, and add_essential_files can be called directly from the command line. Thanks for caching and fixing.

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