-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Changed objects files extension to "obj" on windows #38876
Changed objects files extension to "obj" on windows #38876
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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. |
@@ -109,6 +109,7 @@ impl OutputType { | |||
OutputType::Bitcode => "bc", | |||
OutputType::Assembly => "s", | |||
OutputType::LlvmAssembly => "ll", | |||
OutputType::Object if cfg!(target_os = "windows") => "obj", | |||
OutputType::Object => "o", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this not trigger a dead code warning for one of the two arms depending on target_os
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel right, I'd expect to use the target OS not the host one (of the rustc in use).
That is, this won't use .obj
for cross-compilation and that seems wrong to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd liked to pass Session
and use the sess.target.target.options.target_family
, but Session
is not Sync
, and It cannot be sent in run_work_multithreaded
.
I wonder how should I pass that information (because it will has to be sent to many functions, so I want it to be part of something like WorkItem
or CodegenContext
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be able to clone TargetOptions
(or whatever the type is called)? That or pre-compute the file names. cc @rust-lang/compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddyb Oh, it could be part of TargetOptions
. Is there a reason that Object
is part of OutputType
, and TargetOptions
(which has exe_suffix
, staticlib_suffix
...) does not simply have obj_suffix
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so? Those suffixes seem like the natural place for this though.
The appropriate fix would be to add a field to the target options, similar to the |
Thanks for the PR @liranringel! I fear, though, that the ship may have sailed on this at this point. Although "obj" may be the more correct suffix, today object files are emitted with "o" and tools can be relying on that. I fear that changing this will be a breaking change that would be difficult to work with externally. With that breakage, and with what I believe aren't strong technical reasons to make this change, I'm tempted to leave as-is. |
Ok, then we should close the issue. |
Also, this only needs to produce a |
@liranringel ok I'll close this for now. @retep998 want to track that on the tracking issue? |
I updated the tracking issue with more information. |
Solves this issue:
#37207