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

Official API for build scripts #12432

Closed
epage opened this issue Aug 1, 2023 · 18 comments · Fixed by #14786
Closed

Official API for build scripts #12432

epage opened this issue Aug 1, 2023 · 18 comments · Fixed by #14786
Labels
A-build-scripts Area: build.rs scripts A-cargo-api Area: cargo-the-library API and internal code issues C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review T-cargo Team: Cargo to-announce

Comments

@epage
Copy link
Contributor

epage commented Aug 1, 2023

Problem

Having a Rust API would help with several problems

Proposed Solution

A crate that provides an API for working with build scripts

Notes

Things to work out

As for timing, it'd help if we had this to recommend before we fully resolve #11461 so we can tell people to migrate once, rather than twice

FCP

@epage epage added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` A-cargo-api Area: cargo-the-library API and internal code issues S-triage Status: This issue is waiting on initial triage. labels Aug 1, 2023
bors added a commit that referenced this issue Aug 24, 2023
fix: Set MSRV for internal packages

### What does this PR try to resolve?

Correctly communicates the MSRV we support to our users.

For packages that are more generally mean to be used by other people, I've punted on for now (see ehuss' comment on this PR).  We'll likely need to figure this out for #12432 though

### Additional information

This is prep for a future change which will have us use a fixed rust version for the semver tests with a PR updating them.
bors added a commit that referenced this issue Aug 24, 2023
fix: Set MSRV for internal packages

### What does this PR try to resolve?

Correctly communicates the MSRV we support to our users.

For packages that are more generally mean to be used by other people, I've punted on for now (see ehuss' comment on this PR).  We'll likely need to figure this out for #12432 though

### Additional information

This is prep for a future change which will have us use a fixed rust version for the semver tests with a PR updating them.
@epage epage added the A-build-scripts Area: build.rs scripts label Sep 20, 2023
@ehuss ehuss added S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-triage Status: This issue is waiting on initial triage. labels Sep 25, 2023
bors added a commit that referenced this issue Oct 8, 2023
Set and verify all MSRVs in CI

### What does this PR try to resolve?

Allow us to advertise an MSRV for all public packages, unblocking #12432.

Packages are broken down into two MSRV policies
- Internal packages: `rust-version=stable`
- Public packages: `rust-version=stable-2`

To support this
- RenovateBot will automatically update all MSRVs
- CI will verify all packages build according to their MSRV

Since this takes the "single workspace" approach, the downside is that common dependencies are subject to each package's MSRV.  We also can't rely on `-Zmsrv-policy` to help us generate a lockfile because it breaks down when trying to support multiple MSRVs in a workspace

### How should we test and review this PR?

Per commit

### Additional information

#12381 skipped setting some MSRVs because we weren't sure how to handle it.  For `cargo-credential`, we needed to do something and did one-off verification (#12623).  `cargo-hack` recently gained the ability to automatically select MSRVs for each package allowing us to scale this up to the entire workspace.

I don't know if we consciously chose an MSRV policy for `cargo-credential` but it looked like N-2 so that is what I stuck with and propagated out.
- Without an aggressive sliding MSRV, we discourage people from using newer features because they will feel like they need permission which means it needs to be justified
- Without an aggressive sliding MSRV, if the MSRV at one point in time works for someone, they tend to assume it will always work, leading to frustration at unmet expectations.

I switched the MSRV check to `cargo check` from `cargo test` because I didn't want to deal with the out-of-diskspace issues and `check` will catch 99% of MSRV issues.

Potential future improvements to `cargo-hack`
- Allow `--version-range ..stable` so we can verify all MSRVs that aren't stable which would bypass the diskspace issues and allow us to more easily use `cargo test` again
- Verify on a `cargo package` version of a crate (taiki-e/cargo-hack#216)
@epage
Copy link
Contributor Author

epage commented Oct 9, 2023

FYI we now have an MSRV policy as of #12654

@codyps
Copy link
Contributor

codyps commented Oct 13, 2023

I'm the person who wrote build-env (and also added the target specific env var naming pattern it uses to the cc crate originally):

The content of build-env was primarily written to allow build scripts to implement target specific env variable reading in a way that matches the cc crate's methodology. In practice though, this has been either reimplemented in other build scripts either in the same way (rarely) or in a manner closer but not the same as the style used by cargo's own target specific env variables.

A build-rs-like API for emitting the directives read by cargo or parsing variables set by cargo seems fine, and I expect this crate could use it easily (if I ever update it). I generally like the idea of having a real API instead of peeking into env variables and printing magic strings.

Some encouragement on increasing the scope though:

I'd worry that not providing some API for external info not known to cargo that is still target specific means that the ecosystem of crates will continue to have various inconsistent ways of providing configuration that is (sometimes) target specific to build scripts. As long as env variables are the only really way to communicate that info (ie: until/unless target specs or some target spec adjacent thing exists to do something similar), we're going to want some pattern in env variable naming that tries to obtain consistency. Given that cargo has its own pattern here (<env>_<uppercase target name with underscores>), it could be sensible to adopt it or something based on it. I don't think not having this should stop shipping some sort of build script helper crate, but given the widespread need for something like this, it's something we'll want to include.

Also: just having something that reads an env var and emits the rerun-if-env-changed directive seems like a fairly basic ask, and is something that I've seen duplicated in tons of build scripts. It's so small, that folks are unlikely to pull in another crate to handle it, but as long as we're adding a crate to enable build script writing, it's something that should be included. (Perhaps this is part of what is meant by "A higher level API to help with re-run-if-changed"?)

@madsmtm
Copy link
Contributor

madsmtm commented Jan 11, 2024

Could we consider providing such a crate with rustup? Similarly to how we have the built-inproc_macro crate (I know that one needs to be distributed with the compiler, so maybe a bit different).

@epage
Copy link
Contributor Author

epage commented Jan 12, 2024

rust-lang/compiler-team#659 could allow optional packages in the sysroot but I don't think thats a route we should go.

Being in the sysroot / distributed with rustup means we can't break compatibility. There are some existing packages in this space but I'm not ready to lock us down for 1.0 for ever at this point, if for no other reason.

@madsmtm
Copy link
Contributor

madsmtm commented Jan 12, 2024

not ready to lock us down for 1.0

Totally get that, we'd likely need years of experimentation first, I'm mostly arguing considering that as the end goal.

Just speaking personally, I wouldn't use such a crate, since I'm very wary about compile-times, and the extra dependency doesn't seem worth it for a (percieved) small usability improvement. But if I knew that it was on the way to being part of the standard distribution, I'd be willing to take the compile-time perf hit in the meantime.

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Jun 22, 2024

Hi, at the risk of overstepping my boundaries, I’d like to initiate a repo for this. If I develop an API crate under my own repo could we arrange for it to be subject to code review and if accepted, transferred to the rust-lang organization?

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Jun 22, 2024

Or alternatively we could just add a folder to the cargo repo.

@epage
Copy link
Contributor Author

epage commented Jun 22, 2024

We have a crate name / implementation that was planned to be donated but its stuck in the current owner being busy and me being unfamiliar with the process for migrating something like that.

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Jun 24, 2024

Well build-rs, the crate that you seem to be talking about, does have extremely permissive licensing.

image

With that being the case I doubt they'd be upset about me taking the initiative and adding it to the cargo repository. I think there's real utility in developing it alongside cargo, PRs that modify the API and cargo at the same time won't require two separate PRs and thus can be easier to track. I'm not in a position to talk about how we might gain publishing rights for this crate on crates.io. It's possible that's all we really need though.

@CAD97 I'm pinging you to give you an opportunity to give us some input here. It's not my intent to override your will or governance of your project, but I do think my proposal is in line with your goals.

@CAD97
Copy link
Contributor

CAD97 commented Jun 24, 2024

I'm fully on board with cargo adopting build-rs directly. I've put a small amount of effort into trying to figure out what that'd look like but hadn't really gotten anywhere with that yet.

@Eh2406
Copy link
Contributor

Eh2406 commented Jun 25, 2024

Some people can use crates.io admin privileges to magically move ownership for you if and only if you make it clear that you want it. If you explicitly state that you would like to give published permission to the cargo team we (can probably) talk crates.io admins into making that happen without more intervention on your part.

@epage
Copy link
Contributor Author

epage commented Sep 26, 2024

Uh, wow. I just came across the official policy for the team to take on a crate like this (created new or transfered). As this would be an "intentional artifact", apparently, this requires an RFC (rust-lang/rfcs#3505 is how I found out abut this).

Starting a thread on zulip about this.

@epage
Copy link
Contributor Author

epage commented Oct 2, 2024

Per the updated team charter, the addition of new Intentional artifacts does not need an RFC, only an FCP.

@epage epage added the T-cargo Team: Cargo label Oct 2, 2024
@epage
Copy link
Contributor Author

epage commented Oct 2, 2024

I proposed we adopt build-rs as an Intentional artifact of the Cargo team. Alternatively,
we could also start off build-rs as an Experiment artifact but I feel like having that label would hamper our ability to get feedback.

build-rs seems to be a reasonable name, the owner has offered it to us, and has offered to help with maintenance. Per the official policy, publish rights would need to be completely transferred over to the Project once this proposal has been accepted.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 2, 2024

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Oct 2, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Oct 3, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Oct 13, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Oct 13, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@ehuss ehuss added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Oct 22, 2024
@epage
Copy link
Contributor Author

epage commented Nov 5, 2024

I am now an owner of build-rs and have started the transfer to rust-lang, see https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Transfering.20.60build-rs.60.20crate.20to.20rust-lang/near/480779960

epage added a commit to epage/cargo that referenced this issue Nov 5, 2024
This pulls in https://github.com/cad97/build-rs at eb389d1 with the following changes:
- `Cargo.toml` metadata
- Removal of `.github`, `.gitignore`, `Cargo.lock`

We'll need to integrate `test-lib` into our processes but that seemeed
more invasive, so I wanted to leave that for a future PR.

Infra changes are being coordinated in https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Transfering.20.60build-rs.60.20crate.20to.20rust-lang/near/480779960

Context: per [Cargo's charter](https://doc.crates.io/contrib/team.html#decision-process), we approved this transfer in an [FCP](rust-lang#12432 (comment)).

Fixes rust-lang#12432
github-merge-queue bot pushed a commit that referenced this issue Nov 13, 2024
### What does this PR try to resolve?

Fixes #12432

### How should we test and review this PR?

This pulls in https://github.com/cad97/build-rs at
eb389d1 with the following changes:
- `Cargo.toml` metadata
- Removal of `.github`, `.gitignore`, `Cargo.lock`

We'll need to integrate `test-lib` into our processes but that seemed
more invasive, so I wanted to leave that for a future PR.

### Additional information

Infra changes are being coordinated in
https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Transfering.20.60build-rs.60.20crate.20to.20rust-lang/near/480779960

Context: per [Cargo's
charter](https://doc.crates.io/contrib/team.html#decision-process), we
approved this transfer in an
[FCP](#12432 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts A-cargo-api Area: cargo-the-library API and internal code issues C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants