Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Configure hosts separately from targets when --target is specified. #9322
Configure hosts separately from targets when --target is specified. #9322
Changes from all commits
083cc9e
e51d151
0a603fd
46f9541
e24bf92
e797526
62e2fd4
6dcfe51
222f6ea
4994b5f
b8be3af
5338bb2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can an entry in the unstable book get added for this?
(personally I think it's fine if this is gated by
-Zhost-config
too)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.
Added.
Well we require
-Ztarget-applies-to-host
to be enabled if-Zhost-config
is enabled to ensure we appropriately test the stabilization ordering at the moment since we plan to stabilize thetarget-applies-to-host
option before thehost-config
feature.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.
Can you detail more why these should be stabilized separately? I would imagine that there's no need for them to be separate and they should be stabilized at the same time. It seems like it would be odd if we provide a way to configure host/target separately but then we don't provide a way to configure the host.
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.
Reason is that we don't want to stabilize
host-config
without the new defaults since that complicates testing logic, so we first stabilizetarget-applies-to-host
so that everyone can choose what behavior they want(retain existing or use new) ahead of the default change. For common use cases the host tables aren't actually needed sotarget-applies-to-host
should usually be sufficient anyways to make cross compilation usable with cargo in most cases.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.
@alexcrichton This came out of discussions about the path to future stabilization, and the path to changing the defaults, and how those will interlock.
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.
The issue is that the implementation for one of these features(the
[host]
table feature) depends on the new defaults effectively. The -Z flag fortarget-applies-to-host
does not change the defaults, however the -Z flag forhost-config
does as the host table implementation needs the new defaults.Well they are deliberately separated due to the dependency of the
host
table feature on the new defaults essentially, so they would be stabilized at different times as outlined here. We have essentially one flag for the first step(target-applies-to-host
setting) and one for the other(host
table) with enforced ordering.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
[host]
depends on new defaults, it just depends ontarget-applies-to-host = false
. That can happen either with new defaults or the variable itself. Personally I don't think this is a reason to have two features.To reiterate my thoughts from before, I disagree that these two features should be stabilized separately. They're pretty intertwined and all the hard stuff with one is still present for the other.
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.
Sure.
That by itself isn't the reason for having the features separated, it's a testing issue mostly for both automated tests and those testing with the
-Z
flag, allowing the use of ahost-config
feature with the old defaults increases the combinations we need to test for and results in confusing/unclear behavior such as the host table being ignored if the old defaults are used(in the case thattarget-applies-to-host
is not set before the default change). In addition we want to test the new default behavior as well so tying the new defaults to thehost-table
feature flag seems logical there as then either the[host]
table or the new defaults can be tested with the-Z
host-config
flag together.To some degree they are intertwined but it's mostly a one way dependency,
[host]
depends ontarget-applies-to-host = false
but the opposite isn't really true, if we're going to change the defaults in the future it ends up being a lot easier/clearer to just tie that to the[host]
table feature to simplify testing of the new defaults and avoid any unclear behavior for some combinations. Keep in mind also that the[host]
table feature should only be needed for extremely exotic use cases, thetarget-applies-to-host
feature should be sufficient to fix the broken builds in nearly all cases I can think of so I think it's fine delaying the[host]
table feature stabilization to simplify the migration testing.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.
As I've said many times before, I do not think it is useful to split up these features. I do not think that we can stabilize them on separate schedules, also as I've mentioned before.
This means, to me, the only two states of Cargo that we care about are both features on and both features off. Having one feature on and one off is not a useful state in Cargo since we'll never actually ship that to users. This, to me, means it's not worthwhile to test. It's testing functionality that no one should use, so I don't think we should even expose it and that would simplify things.
I'm honestly not really entirely certain why you're insistent on keeping these separate? As a maintainer of Cargo I'm offering my thoughts to help guide this implementation, but you seem very resolute in your ways that you want precisely the PR as-is right now. I don't really know how to reconcile that honestly. I've tried to respond to all the points you're bringing up but the discussion seems to be very circular where it's just coming around to the same points again and again.
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 guess I'm missing what the issue with this is I guess.
There's still the default change as well, which I also wanted to test, sure, we could combine the others and separate that if you want or just not test the new defaults I supposed but that seemed to me to result in a more confusing migration process as only allowing the host config table feature with the new defaults seem to simplify testing of that feature+default combination.
Well after the new defaults are applied one shouldn't need to set
target-applies-to-host = false
ever to usehost
tables, so I was just trying to avoid needing to settarget-applies-to-host = false
in cases where one also sets a host table as we probably won't have as good test coverage of all the combinations, we'll still probably be fine but I am less confident in regards to what the implementation correctness would be with them combined and having the default be introduced after thehost
table feature.I'm just trying to clarify the issues I'm running into as it's unclear what the best approach to dealing with them in a satisfactory way is.
Well we inherently have two sets of changes either way, the
target-applies-to-host
feature needs to be introduced as part of the first, sure we could introduce ahost
table at the same time as that but we still have the default change down the line anyways so that's why I tied the new defaults to thehost
table to just have those both as part of the second set of changes.It's unclear how you want me to implement the change though, is either of these approaches what you have in mind:
target-applies-to-host
tohost
table feature and don't bother testing new defaults at all.target-applies-to-host
tohost
table feature and add a different-Z
flag which enables the new defaults for testing.I think the existing approach is less confusing but either of those would still work fine if that's what you think is best.