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

support generic target tables and env variables #9603

Closed
wants to merge 1 commit into from

Conversation

jameshilliard
Copy link
Contributor

When building for targets from a meta build system like buildroot it is preferable to be able to unconditionally set target config/env variables without having to care about the target triple as we use target specific toolchains that will only support a single target architecture typically.

@rust-highfive
Copy link

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2021
@jameshilliard jameshilliard force-pushed the generic-config branch 15 times, most recently from dea5a53 to b794b44 Compare June 24, 2021 00:14
@bors
Copy link
Contributor

bors commented Aug 23, 2021

☔ The latest upstream changes (presumably #9821) made this pull request unmergeable. Please resolve the merge conflicts.

@jameshilliard jameshilliard force-pushed the generic-config branch 2 times, most recently from 6651105 to ef6d87b Compare December 27, 2022 21:39
When building for targets from a meta build system like buildroot it
is preferable to be able to unconditionally set target config/env
variables without having to care about the target triple as we use
target specific toolchains that will only support a single target
architecture typically.
@bors
Copy link
Contributor

bors commented Feb 21, 2023

☔ The latest upstream changes (presumably #11745) made this pull request unmergeable. Please resolve the merge conflicts.

@epage
Copy link
Contributor

epage commented Jul 18, 2024

Sorry for such a late response on this. I don't quite know our practices when this was opened but we now ask that people start with issues discussing their problem and potential solutions for us to make sure we are all on the same page on what the problem is and why we should go with a specific solution, before moving onto the implementation, so we stay focused on the right level.

As this is several years stale at this point, I'm going to go ahead and close this. If someone wants t pick this back up, the first step would be to create an issue.

@jameshilliard
Copy link
Contributor Author

jameshilliard commented Jul 18, 2024

I don't quite know our practices when this was opened but we now ask that people start with issues discussing their problem and potential solutions for us to make sure we are all on the same page on what the problem is and why we should go with a specific solution, before moving onto the implementation, so we stay focused on the right level.

Is it normal for pull requests to just get ignored for years? Is cargo still being maintained?

Cargo's cross compilation has been broken without ugly hacks for years due to no progress on #9753.

@epage
Copy link
Contributor

epage commented Jul 18, 2024

In 2022 we announced what was effectively happening

As a result of these changes to the team, the rate of new PRs is beyond our capacity to accept at this time. Reviews for PRs will be taking significantly longer than before. For now, Cargo will be having a freeze on any new features or major changes. We will still be accepting bug fixes and work on existing projects under active development. As capacity becomes more available, new features may be accepted after being approved by the Cargo Team.

Since then, we've added more team members and we've been able to pick up more work. We are still limited and to better manage the ours and contributors time, we've put more of an emphasis on the design phase being in issues.

@jameshilliard
Copy link
Contributor Author

Alex Crichton has done more than his fair share being a keystone holding the Rust project together. Several years ago he stepped back from single-handedly running everything, to spin out many new teams to take care of things he did alone.

Hmm, I think Alex was the main reviewer for the cross compilation related PRs I was working on, guess that somewhat explains why things stalled out.

Since then, we've added more team members and we've been able to pick up more work.

Is there a new maintainer reviewing cross compilation PRs currently, or is there no maintainer currently that actually understands cargo cross compilation well enough to review/merge cross compilation related PRs?

If that's the case maybe it makes sense to rely more on external contributor review from those who are experts in cross compilation for guidance. The cargo development process seems rather opaque at the moment for external contributors.

I mean it seems to me that cross compilation support in cargo just isn't really being maintained much anymore if one still can't get non-broken cross compilation behavior on a stable toolchain without setting scary looking env variables like __CARGO_TEST_CHANNEL_OVERRIDE_DO_NOT_USE_THIS.

We are still limited and to better manage the ours and contributors time, we've put more of an emphasis on the design phase being in issues.

So is there a plan for reviewing/merging PRs like #9753 at some point? There's over 100 comments relating to the design of that flag in #9322 already.

target-applies-to-host=false behavior is also not a minor nice to have feature but rather a hard requirement for cross compilation to be functional at all for a number of common build scenarios(i.e. building binaries on a x86_64 build host for a x86_64 target).

This PR is somewhat related to my other cross compilation PRs in which there are extensive related discussions across various issues/PRs.

@epage
Copy link
Contributor

epage commented Jul 18, 2024

Is there a new maintainer reviewing cross compilation PRs currently, or is there no maintainer currently that actually understands cargo cross compilation well enough to review/merge cross compilation related PRs?

I'm not seeing any S-accepted cross-compilation Issues, meaning no team member has committed to having review capacity to take them on.

If that's the case maybe it makes sense to rely more on external contributor review from those who are experts in cross compilation for guidance. T

Having other people look at the reviews to help with details is useful but, in the end, the author and/or reviewers will need to be able to explain the user-facing side of the change to the cargo team for them to sign off on any compatibility commitments.

The cargo development process seems rather opaque at the moment for external contributors.

We have https://doc.crates.io/contrib/

Mentioned in there is https://github.com/rust-lang/cargo/wiki/Office-Hours

If there is something left opaque from those, let us know!

I mean it seems to me that cross compilation support in cargo just isn't really being maintained much anymore if one still can't get non-broken cross compilation behavior on a stable toolchain without setting scary looking env variables like __CARGO_TEST_CHANNEL_OVERRIDE_DO_NOT_USE_THIS.

I feel that is an over-generalization as I believe people are using Cargo for cross-compilation without relying on internal features.

So is there a plan for reviewing/merging PRs like #9753 at some point?

I don't remember the details from when that was last discussed but I believe there were major concerns that needed addressing. And, like I said, if there isn't someone willing to commit the time (which I at least don't have), then it will remain stalled out. There are many people coming to us with many concerns and we can't address everything.

@jameshilliard
Copy link
Contributor Author

I'm not seeing any S-accepted cross-compilation Issues, meaning no team member has committed to having review capacity to take them on.

Hmm, doesn't seem that S-accepted flag is being used for some target-applies-to-host related cross compilation PRs like #13900 which was merged and #14206 which is currently pending.

I feel that is an over-generalization as I believe people are using Cargo for cross-compilation without relying on internal features.

So, the broken cross compilation behavior doesn't always cause builds to fail(i.e. if cross compilation target binaries happen to be compatible with the host userspace), but AFAIU it is not possible to actually fix the broken behavior without setting the nightly target-applies-to-host=false flag. Essentially the default cross compilation behavior is broken but happens to work by accident in a number of scenarios, which is likely why this fundamentally broken behavior was not discovered when the config system was originally designed. I don't think there was any real disagreement that the existing behavior is broken AFAIU.

We have https://doc.crates.io/contrib/

Mentioned in there is https://github.com/rust-lang/cargo/wiki/Office-Hours

If there is something left opaque from those, let us know!

I think relying on things like office hours may be why cargo's development process seems so opaque to me. If a lot of decision making is happening in ephemeral conversations without being communicated somewhere else it can be difficult for external contributors to understand maintainer decisions/background.

I don't remember the details from when that was last discussed but I believe there were major concerns that needed addressing.

I mean, I have no idea what the actual issue was holding up stabilizing this like, if it's related to some bugs like #14206 or something else? AFAIU there were some private/semi-private discussions around it but I couldn't find any public details regarding what the blocking issues were.

And, like I said, if there isn't someone willing to commit the time (which I at least don't have), then it will remain stalled out. There are many people coming to us with many concerns and we can't address everything.

The impression I'm getting from reading the docs is that the cargo maintainers want to strongly discourage any external code contributions(at least for issues other than ones they specifically care about), I guess that's a policy change due to a lack of maintainers? External contributors will often only be able to contribute in areas that are within their domain expertise. The way the development process is currently set up also seems to not be very scalable if there's a bunch of individual mentoring required before a contributor even submits code for review. Most open source projects seem to prefer pull requests with concrete proposed fixes vs issues, it seems very strange that cargo is different there.

@weihanglo
Copy link
Member

Hmm, doesn't seem that S-accepted flag is being used for some target-applies-to-host related cross compilation PRs like #13900 which was merged and #14206 which is currently pending.

That was my fault forgetting to put the label on. These are bug-fix like PRs. Some code paths were panicking. And the author has clearly demonstrated the behavior change with a table, so it's a bit easier for me to just review and merge a "bugfix". Even so it still took me more time than expected to review.

For design and stabilization, I myself might not have energy to drive. Somebody needs to propose a clear plan, especially about use cases and breakages and how to deal with them. Then some contributors can look into it and maybe later collaborate on an RFC.

@epage
Copy link
Contributor

epage commented Jul 19, 2024

To add, our bar for stuff behind feature gates is a lot lower.

@epage
Copy link
Contributor

epage commented Jul 19, 2024

I think relying on things like office hours may be why cargo's development process seems so opaque to me. If a lot of decision making is happening in ephemeral conversations without being communicated somewhere else it can be difficult for external contributors to understand maintainer decisions/background.

Mentorship Office Hours is not where decision making is happening but where we offer synchronous communication to help people make progress.

Most informal decision making happens in Cargo team meetings. We try to communicate out the discussions on relevant issues (e.g. #5707). Otherwise, the "This Development-cycle in Cargo" blog series are meant to help communicate out the rest (example). The subject of discussion is generally what we are willing to commit to for review.

The impression I'm getting from reading the docs is that the cargo maintainers want to strongly discourage any external code contributions(at least for issues other than ones they specifically care about), I guess that's a policy change due to a lack of maintainers? External contributors will often only be able to contribute in areas that are within their domain expertise.

We have to acknowledge what we have the bandwidth for to prevent burnout. There are many people with many interests and there are not enough of us to handle them all.

The way the development process is currently set up also seems to not be very scalable if there's a bunch of individual mentoring required before a contributor even submits code for review.

The project can only scale up to review capacity. Unsure what you are getting at about needing a lot of individual mentoring.

Most open source projects seem to prefer pull requests with concrete proposed fixes vs issues, it seems very strange that cargo is different there.

  • Frequently, there are multiple attempts to fix a single problem. By being PR-first, the conversation gets scattered and lost.
  • By being Issue first, we avoid the frustration of a contributor putting in a lot of work for us to say "this was the wrong direction". Conversely, it takes the social pressure of a contributor putting in a lot of work off of us.

@jameshilliard
Copy link
Contributor Author

These are bug-fix like PRs. Some code paths were panicking. And the author has clearly demonstrated the behavior change with a table, so it's a bit easier for me to just review and merge a "bugfix".

So, for something like #9753 which is basically just stabilizing a flag that allows people to disable a cargo bug(it's a flag due to backwards compatibility concerns), should that be marked as a bugfix?

Most informal decision making happens in Cargo team meetings.

I think this is probably why the process seems so opaque to outside contributors.

We try to communicate out the discussions on relevant issues (e.g. #5707).

So the problem I see with this process is pretty well demonstrated by that specific example in that when decisions are made only the final decision seems to be getting communicated outside of the meeting. The reasoning behind the decision then often gets lost effectively, how important this is varies but losing that information is often quite problematic. Losing decision context due to the background information being ephemeral can be especially problematic if say some assumptions behind a decision were wrong or for long running issues where losing this information can lead to additional confusion down the line.

The project can only scale up to review capacity.

Maybe there's some changes that need to be made in order to scale up review capacity? Is there a lack of people with appropriate capabilities that are willing to do reviews?

Unsure what you are getting at about needing a lot of individual mentoring.

I think it's better to focus on say ways to make it easier for contributors to come up with solutions on their own that are in line with what the maintainers want wants. If you have a scaling issue doesn't it make sense to try and eliminate bottlenecks like having to wait for maintainers to have time to go over issue background?

By being Issue first, we avoid the frustration of a contributor putting in a lot of work for us to say "this was the wrong direction".

So if say someone has a critical bug with cargo, like #3349 which failed to get meaningful interest from cargo developers(maybe due to cargo developers having difficulty understanding the context of the issue) what would you suggest someone do? I mean since that issue was going nowhere I came up with an initial fix on my own in #9322. If I hadn't proposed a concrete solution to the issue it seems no progress would have been made at all on a fix.

By being Issue first, we avoid the frustration of a contributor putting in a lot of work for us to say "this was the wrong direction".

If someone hits a bug they need to fix and there's no movement/interest from cargo maintainers after filing an issue the person affected by the bug is likely going to have to put in work to fix it anyways. Once they have a potential fix I would think it would make sense to propose the fix as a solution to the issue at that point. For example the buildroot project which I contribute to has a policy that any package patches should be submitted to the upstream project before being included in buildroot, unless the patch is clearly not applicable to the upstream project.

Conversely, it takes the social pressure of a contributor putting in a lot of work off of us.

Discouraging external code contributions for fixes is effectively encouraging forks/fragmentation and downstream hacks to work around issues/bugs, at least if you encourage pull requests(even if they take forever to merge) others running into the same/similar issues may find those open pull requests so that less work is being duplicated.

@epage
Copy link
Contributor

epage commented Jul 23, 2024

So, for something like #9753 which is basically just stabilizing a flag that allows people to disable a cargo bug(it's a flag due to backwards compatibility concerns), should that be marked as a bugfix?

That is a dramatic oversimplification. In one case, it is fixing a panic in unstable code. In the other case, a whole new user-facing, backwards-compatible feature is being added. It was also up to that reviewer's discretion to look over that code. Individual reviewers do not owe you anything.

 So the problem I see with this process is pretty well demonstrated by that specific example in that when decisions are made only the final decision seems to be getting communicated outside of the meeting. 

The decision was for the flag. The rest of the information was notes to help someone unfamiliar with expectations. There are policies implied by those steps but those are or align with pre-existing policies.

And yes, sometimes I don't think to write up enough details. In particular, my target audience was someone who wanted it implemented and for their use case, there was little to discuss. Anything more to discuss is likely a superset of what is talked about there anyways. The blog posts (which you didn't address) tend to go into more detail and I try to cover every Cargo team meeting topic. I take 2-3 full-time days to write each post.

Maybe there's some changes that need to be made in order to scale up review capacity? Is there a lack of people with appropriate capabilities that are willing to do reviews?

Cargo has a wide but shallow "API" surface that we have to maintain compatibility on. We need reviewers that we are comfortable with identifying when things touch on backwards compatibility, requiring pulling in the Cargo team to make a decision on it. Even knowing whether a "bugfix" would break existing users is hard to tell.

That doesn't mean we aren't trying to improve things, When we did the soft-feature freeze, one of our priorities was finding ways to make it easier to be a maintainer and easier to contribute. Some recent examples include:

We are also working to better modularize Cargo so it is easier to reason about.

unless the patch is clearly not applicable to the upstream project.

I think it depends on what their level of policy is on this. There are a lot of cases where people solved a problem for themself but it isn't a viable solution for all users and the PR gets reject. One of the goals of this approach is to vet these ideas.

Discouraging external code contributions for fixes is effectively encouraging forks/fragmentation and downstream hacks to work around issues/bugs, at least if you encourage pull requests(even if they take forever to merge) others running into the same/similar issues may find those open pull requests so that less work is being duplicated.

This again assumes that PRs will get merged as-is.

This also assumes there is no cost to keeping open PRs, both to the author and the Cargo team.

@jameshilliard
Copy link
Contributor Author

In one case, it is fixing a panic in unstable code. In the other case, a whole new user-facing, backwards-compatible feature is being added.

Note that target-applies-to-host is effectively a feature gated bug fix. Setting target-applies-to-host=false fixes a fatal logic bug in the stable code.

The blog posts (which you didn't address) tend to go into more detail and I try to cover every Cargo team meeting topic. I take 2-3 full-time days to write each post.

This is probably not going to work well as a way to communicate with external contributors, at least for those not otherwise involved in cargo development as much of the information on the blog will not be relevant to what they are working on.

For example I do not really develop any rust code myself at the moment or have much involvement in the cargo/rust ecosystem beyond fixing cross compilation toolchain/build issues in various rust/cargo packages, rust has become a common dependency for many python packages and some system packages that I use so it's important for me that cargo actually has properly functional cross compilation(which is definitely not the case without enabling nightly features). Beyond fixing bugs or build system integration issues I'm not particularly interested in the day to day feature development of cargo.

We are also working to better modularize Cargo so it is easier to reason about.

I mean, there's some edge case issues but I don't think that was really the main problem I was seeing, I was able to come up with an initial approach to fixing this issue without even having any real background using rust in general.

I think it depends on what their level of policy is on this. There are a lot of cases where people solved a problem for themself but it isn't a viable solution for all users and the PR gets reject. One of the goals of this approach is to vet these ideas.

So buildroot for example would want a PR opened upstream for the target-applies-to-host fix because it's an issue that's clearly relevant to many other cross compilation use cases beyond the buildroot project. Normally we can carry downstream patches but it is quite difficult with cargo since a rust toolchain is non-trivial to bootstrap from source so we're mostly using the binary packages as part of our toolchain.

This again assumes that PRs will get merged as-is.

Not really, I'm just assuming that it's a good idea to have a place for say downstream integrators to collaborate on fixes even in cases where maintainers don't necessarily have time to immediately review the PR.

This also assumes there is no cost to keeping open PRs, both to the author and the Cargo team.

I'm mostly referring to cases where it's clear that the issue a PR is addressing is something that needs to eventually be fixed. External contributors that way have an obvious place in which they can collaborate on improvements to the PR so that by the time Cargo maintainers have time it may be in a much better state.

@epage
Copy link
Contributor

epage commented Jul 23, 2024

Note that target-applies-to-host is effectively a feature gated bug fix. Setting target-applies-to-host=false fixes a fatal logic bug in the stable code.

I clarified the difference and it was grossly overlooked. I suspect at this point, some space is needed on this conversation so we can better come to it fresh.

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.

6 participants