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

Assume `build.rs` in the same directory as `Cargo.toml` is a build script (unless explicitly told not to) #3361

Merged
merged 5 commits into from Dec 7, 2016

Conversation

Projects
None yet
6 participants
@pwoolcoc
Contributor

pwoolcoc commented Dec 2, 2016

So, in May I posted a question in the #cargo IRC room: https://botbot.me/mozilla/cargo/2016-05-26/?msg=66770068&page=1

This PR does what was discussed there. If cargo sees a build.rs in the same directory as the Cargo.toml, it will assume build.rs is a build script unless package.build = false. Just for completeness I also made build = true mean the same as build = "build.rs" but I'm not sure if that is okay or not.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 2, 2016

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

pwoolcoc added some commits Dec 2, 2016

Assume build = 'build.rs' by default
This change makes cargo assume `build = "build.rs"` if there is a
`build.rs` file in the same directory as `Cargo.toml`. However, you
can set `build = false` to prevent this.
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 2, 2016

Member

yessssssss i asked @alexcrichton about this a while back and he was 👍 at the time. I think this is great.

Member

steveklabnik commented Dec 2, 2016

yessssssss i asked @alexcrichton about this a while back and he was 👍 at the time. I think this is great.

Show outdated Hide outdated src/cargo/util/toml.rs
@@ -540,7 +546,11 @@ impl TomlManifest {
}
// processing the custom build script
let new_build = project.build.as_ref().map(PathBuf::from);
let manifest_file = util::important_paths::find_root_manifest_for_wd(None, &layout.root)

This comment has been minimized.

@pwoolcoc

pwoolcoc Dec 2, 2016

Contributor

Looking this over again, I'm wondering if I actually have to do this, or if I can just assume base_dir == layout.root?

@pwoolcoc

pwoolcoc Dec 2, 2016

Contributor

Looking this over again, I'm wondering if I actually have to do this, or if I can just assume base_dir == layout.root?

@pwoolcoc

This comment has been minimized.

Show comment
Hide comment
@pwoolcoc

pwoolcoc Dec 2, 2016

Contributor

@steveklabnik :) everyone who commented in #cargo seemed to support it, but it was a while ago, so I'm hoping the support is still there.

Contributor

pwoolcoc commented Dec 2, 2016

@steveklabnik :) everyone who commented in #cargo seemed to support it, but it was a while ago, so I'm hoping the support is still there.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 2, 2016

Member

Thanks for the PR! The implementation looks great to me as well.

My only hesitation here is that we risk breaking projects when this lands. Projects can fix builds with build = false in Cargo.toml, but I think that'll end up getting rejected on the stable/beta channels. In that sense, if a crate has a build.rs but it's not a build script, then you can't keep that layout and continue to work on stable/beta/nightly.

I'd also want to think through the backcompat story here with all crates published on crates.io. It may be worth doing a survey there to see how many crates have a build.rs file which isn't a build script, if any. If that number is 0, then our lives are much easier! If that number is greater, we may want to proactively reach out to authors to head off the breakage.

@pwoolcoc would you be willing to scrape crates.io and test for build.rs scripts in roots which aren't build scripts?

Member

alexcrichton commented Dec 2, 2016

Thanks for the PR! The implementation looks great to me as well.

My only hesitation here is that we risk breaking projects when this lands. Projects can fix builds with build = false in Cargo.toml, but I think that'll end up getting rejected on the stable/beta channels. In that sense, if a crate has a build.rs but it's not a build script, then you can't keep that layout and continue to work on stable/beta/nightly.

I'd also want to think through the backcompat story here with all crates published on crates.io. It may be worth doing a survey there to see how many crates have a build.rs file which isn't a build script, if any. If that number is 0, then our lives are much easier! If that number is greater, we may want to proactively reach out to authors to head off the breakage.

@pwoolcoc would you be willing to scrape crates.io and test for build.rs scripts in roots which aren't build scripts?

@pwoolcoc

This comment has been minimized.

Show comment
Hide comment
@pwoolcoc

pwoolcoc Dec 3, 2016

Contributor

@alexcrichton sure, I can do that.

Would it preferable to change it slightly to only print a deprecation warning when there is a build.rs that isn't a build script, and land the rest of the change after a couple releases?

Contributor

pwoolcoc commented Dec 3, 2016

@alexcrichton sure, I can do that.

Would it preferable to change it slightly to only print a deprecation warning when there is a build.rs that isn't a build script, and land the rest of the change after a couple releases?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 3, 2016

Member
Member

steveklabnik commented Dec 3, 2016

@pwoolcoc

This comment has been minimized.

Show comment
Hide comment
@pwoolcoc

pwoolcoc Dec 3, 2016

Contributor

@steveklabnik yea, if someone has a crate with a build.rs file in the same directory as Cargo.toml, and the build.rs is not a build script, then this patch will try to treat it as a build script. The user would have to add build = false to the package section of their Cargo.toml in order to prevent this.

Contributor

pwoolcoc commented Dec 3, 2016

@steveklabnik yea, if someone has a crate with a build.rs file in the same directory as Cargo.toml, and the build.rs is not a build script, then this patch will try to treat it as a build script. The user would have to add build = false to the package section of their Cargo.toml in order to prevent this.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 3, 2016

Member

@pwoolcoc

Would it preferable to change it slightly to only print a deprecation warning when there is a build.rs that isn't a build script, and land the rest of the change after a couple releases?

If we have no existing build.rs scripts that aren't build scripts, I think we can avoid this. Otherwise yeah I think we'll need to take this route. I figure we should just get a handle on the situation first so we can categorize our options.

@steveklabnik

the way in which this would break is if someone had specially configured a build.rs at the top level to be source, correct?

Indeed! I agree that it should be rare if ever, but I just want to make sure!

Member

alexcrichton commented Dec 3, 2016

@pwoolcoc

Would it preferable to change it slightly to only print a deprecation warning when there is a build.rs that isn't a build script, and land the rest of the change after a couple releases?

If we have no existing build.rs scripts that aren't build scripts, I think we can avoid this. Otherwise yeah I think we'll need to take this route. I figure we should just get a handle on the situation first so we can categorize our options.

@steveklabnik

the way in which this would break is if someone had specially configured a build.rs at the top level to be source, correct?

Indeed! I agree that it should be rare if ever, but I just want to make sure!

pwoolcoc added some commits Dec 5, 2016

@pwoolcoc

This comment has been minimized.

Show comment
Hide comment
@pwoolcoc

pwoolcoc Dec 5, 2016

Contributor

@alexcrichton ok, I'm finishing up the few crates that my script couldn't automatically check, but here are the preliminary results. There are 12 crates that have a build.rs in the same directory as a Cargo.toml, without an accompanying build = "build.rs" in the Cargo.toml file. However, all of the build.rs are build scripts that are just not being used. None of them are actually source code for the project. However, this still means that their builds might break from this change.

The 12 crates are:

Contributor

pwoolcoc commented Dec 5, 2016

@alexcrichton ok, I'm finishing up the few crates that my script couldn't automatically check, but here are the preliminary results. There are 12 crates that have a build.rs in the same directory as a Cargo.toml, without an accompanying build = "build.rs" in the Cargo.toml file. However, all of the build.rs are build scripts that are just not being used. None of them are actually source code for the project. However, this still means that their builds might break from this change.

The 12 crates are:

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 6, 2016

Member

@pwoolcoc thanks for the investigation! Looking at those thought I think that this change would break most of those crates unfortunately. Most of the build scripts have an extern crate directive which isn't listed in [build-dependencies].

Given that we may need to pursue a strategy like:

  • Add knowledge of build = <bool>
  • If build isn't specified and build.rs exists, generate a warning

We probably need to let that bake for quite some time, and then after that's in we can enable the auto-detection. What do you think?

Member

alexcrichton commented Dec 6, 2016

@pwoolcoc thanks for the investigation! Looking at those thought I think that this change would break most of those crates unfortunately. Most of the build scripts have an extern crate directive which isn't listed in [build-dependencies].

Given that we may need to pursue a strategy like:

  • Add knowledge of build = <bool>
  • If build isn't specified and build.rs exists, generate a warning

We probably need to let that bake for quite some time, and then after that's in we can enable the auto-detection. What do you think?

@pwoolcoc

This comment has been minimized.

Show comment
Hide comment
@pwoolcoc

pwoolcoc Dec 6, 2016

Contributor

@alexcrichton sounds good, I suspected as much, so I have a patch ready to go for this. So to be clear, the new behavior would be:

  1. If build = "build.rs" is present, same behavior as today
  2. if build = true is present, it is equivalent to build = "build.rs", regardless of the existence of build.rs
  3. If build = false is present, it is equivalent to not having a build value at all, regardless of the existence of build.rs
  4. If build = ... is not present, and a build.rs file is present, a warning would be printed.

Does that sound right? I'll push my last commit so you can look it over, too.

Contributor

pwoolcoc commented Dec 6, 2016

@alexcrichton sounds good, I suspected as much, so I have a patch ready to go for this. So to be clear, the new behavior would be:

  1. If build = "build.rs" is present, same behavior as today
  2. if build = true is present, it is equivalent to build = "build.rs", regardless of the existence of build.rs
  3. If build = false is present, it is equivalent to not having a build value at all, regardless of the existence of build.rs
  4. If build = ... is not present, and a build.rs file is present, a warning would be printed.

Does that sound right? I'll push my last commit so you can look it over, too.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 6, 2016

Member

@pwoolcoc sounds exactly right to me!

Member

alexcrichton commented Dec 6, 2016

@pwoolcoc sounds exactly right to me!

@pwoolcoc

This comment has been minimized.

Show comment
Hide comment
Contributor

pwoolcoc commented Dec 6, 2016

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Dec 6, 2016

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 7, 2016

Member

@bors: r+

Thanks!

Member

alexcrichton commented Dec 7, 2016

@bors: r+

Thanks!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 7, 2016

Contributor

📌 Commit 1b99491 has been approved by alexcrichton

Contributor

bors commented Dec 7, 2016

📌 Commit 1b99491 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 7, 2016

Contributor

⌛️ Testing commit 1b99491 with merge 333a798...

Contributor

bors commented Dec 7, 2016

⌛️ Testing commit 1b99491 with merge 333a798...

bors added a commit that referenced this pull request Dec 7, 2016

Auto merge of #3361 - pwoolcoc:default-build-script, r=alexcrichton
Assume `build.rs` in the same directory as `Cargo.toml` is a build script (unless explicitly told not to)

So, in May I posted a question in the #cargo IRC room: https://botbot.me/mozilla/cargo/2016-05-26/?msg=66770068&page=1

This PR does what was discussed there. If `cargo` sees a `build.rs` in the same directory as the `Cargo.toml`, it will assume `build.rs` is a build script unless `package.build = false`. Just for completeness I also made `build = true` mean the same as `build = "build.rs"` but I'm not sure if that is okay or not.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 7, 2016

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 333a798 to master...

Contributor

bors commented Dec 7, 2016

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 333a798 to master...

@bors bors merged commit 1b99491 into rust-lang:master Dec 7, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@pwoolcoc

This comment has been minimized.

Show comment
Hide comment
@pwoolcoc

pwoolcoc Dec 9, 2016

Contributor

@alexcrichton sorry to revive the old thread, just had a question about this. What is the procedure for going back to finish features like this a few releases down the line? Should I open a tracking issue for it? Or just put a reminder in my calendar for March and ping you again after a couple release cycles?

Contributor

pwoolcoc commented Dec 9, 2016

@alexcrichton sorry to revive the old thread, just had a question about this. What is the procedure for going back to finish features like this a few releases down the line? Should I open a tracking issue for it? Or just put a reminder in my calendar for March and ping you again after a couple release cycles?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 10, 2016

Member

@pwoolcoc oh we unfortunately don't have a super principled way of handling this in Cargo. For now though want to open a tracking issue here?

Member

alexcrichton commented Dec 10, 2016

@pwoolcoc oh we unfortunately don't have a super principled way of handling this in Cargo. For now though want to open a tracking issue here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment