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

Open
epage opened this issue Aug 1, 2023 · 11 comments
Open

Official API for build scripts #12432

epage opened this issue Aug 1, 2023 · 11 comments
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` S-needs-team-input Status: Needs input from team on whether/how to proceed.

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

@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

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

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

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.

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` S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

7 participants