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

Attempt to fix Issue #9451 #10084

Closed
wants to merge 5 commits into from
Closed

Attempt to fix Issue #9451 #10084

wants to merge 5 commits into from

Conversation

kennystrawnmusic
Copy link

Running generate_targets() before running the -Zbuild-std requires --target check should, in theory, allow --per-package-target to play more nicely with -Zbuild-std than it does currently.

Running `generate_targets()` *before* running the `-Zbuild-std requires --target` check should, in theory, allow `--per-package-target` to play more nicely with `-Zbuild-std` than it does currently.
@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.

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 Nov 16, 2021
@kennystrawnmusic
Copy link
Author

So it seems the only failure is in the rustfmt check, not in the builds — which suggests that, although the code isn't formatted the way rustfmt would like after my changes, it's still perfectly functional. Shouldn't be a problem when merging at this point.

@alexcrichton
Copy link
Member

Thanks for the PR! FWIW per-package-target is still "broken" in that the target calculation part of generate_targets needs to be completely moved elsewhere. In any case though I agree that this should fix the immediate issue.

We don't merge PRs unless they're green on CI, so if you could reformat the code with rustfmt I'll r+

@ehuss
Copy link
Contributor

ehuss commented Nov 18, 2021

Can this also get a test?

Copied and pasted from upstream and then, instead of cutting and pasting the other stuff around the problematic check, cut and paste the check itself from the line it was on before down to a later one. Hopefully rustfmt doesn't freak out this time.
@kennystrawnmusic
Copy link
Author

Alright @alexcrichton rustfmt is now successful. All the other checks haven't finished running yet as of this writing but given that they have all succeeded before the CI should be green at this point.

@alexcrichton
Copy link
Member

Will you be able to write a test for this as @ehuss requested?

@kennystrawnmusic
Copy link
Author

Will you be able to write a test for this as @ehuss requested?

Working on it right now

@kennystrawnmusic
Copy link
Author

kennystrawnmusic commented Nov 20, 2021

Update: What I did to test this was attempt to compile this with the manifest file set to use the per-package-target specific Cargo.toml options and had forced-target set to the custom .json file ― yet for some reason, regardless of where this check was in the code, they still weren't being honored. Turns out, in a nutshell, that merely moving this check down further didn't solve the problem.

So I tried instead to see what would happen if I commented out the check in question and then tested the compiler that way. You'd expect forced-target to set the target to that custom one, but it didn't. Instead, the compiler panicked. So I tried using the --target=x86_64-unknown-none.json flag instead to set the target manually from the command line. This time, my fork with the check commented out didn't panic and instead compiled successfully. This suggests that the culprit isn't with the compilation mechanism but rather with the way per-package-target is handled. As a result, I am going to unfortunately have to close this request for now and snoop around in the code some more; when I find some other culprit I'll open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants