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

Verify that src dir wasn't modified by build.rs when publishing #5584

Merged
merged 8 commits into from May 29, 2018

Conversation

Projects
None yet
9 participants
@gferon
Copy link
Contributor

gferon commented May 28, 2018

Fixes issue #5073.

Implemented during RustFest Paris impl days.

boxdot and others added some commits May 28, 2018

Verify that src dir was not modified by build.rs during publish.
Co-authored-by: Gabriel Feron <feron.gabriel@gmail.com>
Add test to check that publish fails when src directory was changed.
Co-authored-by: boxdot <d@zerovolt.org>
@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented May 28, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@matklad
Copy link
Member

matklad left a comment

Excellent!

.build();

assert_that(
p.cargo("publish")

This comment has been minimized.

@matklad

matklad May 28, 2018

Member

We can use cargo package here, to avoid the need to setup the publishing infra.

.arg("--index")
.arg(publish::registry().to_string()),
execs().with_status(101),
);

This comment has been minimized.

@matklad

matklad May 28, 2018

Member

Let's also check that running cargo pacakge --no-verify suppresses this check!

gferon and others added some commits May 28, 2018

Document that build scripts should not modify files outside OUT_DIR.
Co-authored-by: Gabriel Feron <feron.gabriel@gmail.com>
);

assert_that(
p.cargo("package")

This comment has been minimized.

@matklad

matklad May 28, 2018

Member

This can be more compactly written as p.cargo("package --no-verify"), the testing infra gained ability to split arguments automatically fairly recently.

gferon and others added some commits May 28, 2018

Provide more context when src files changed while packaging
Co-authored-by: boxdot <d@zerovolt.org>

@gferon gferon force-pushed the boxdot:master branch from a926a46 to e31f3c3 May 28, 2018

@@ -272,6 +272,11 @@ There’s a couple of points of note here:
output files should be located. It can use the process’ current working
directory to find where the input files should be located, but in this case we
don’t have any input files.
* In general, build scripts should not modify any files outside of `OUT_DIR`.
It may seem fine on the first blush, but it does cause problems when you use

This comment has been minimized.

@boxdot

boxdot May 28, 2018

Contributor

Thanks @matklad for formulating this already in the issue.

bail!(
"Source directory was modified by build.rs during cargo publish. \
Build scripts should not modify anything outside of OUT_DIR. \
Modified file: {}",

This comment has been minimized.

@matklad

matklad May 28, 2018

Member

One more thing: let's add use '--no-verify' flag to suppress this check to the error message?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented May 28, 2018

@alexcrichton, are we comfortable with enabling this behavior by default?

  • this might make some existing crates to fail `cargo publish, which is bad (breaking change of sorts), but at the same time is kind of a point of the feature
  • suppressing the check is easy, one can add --no-verify flag
  • publish is a rare operation, and in general involves human, so the change wouldn't break someone's CI hopefully.

We can down tone this to a warning perhaps? I personally am fine with a hard error.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented May 28, 2018

👍 I'm in favor of enabling this by default

I think the error message should indicate that it can be fixed with the --no-verify flag as @matklad already mentioned but other than that I'm in favor!

@matklad matklad added the relnotes label May 29, 2018

Caused by:
Source directory was modified by build.rs during cargo publish. \
Build scripts should not modify anything outside of OUT_DIR. Modified file: [..]src/generated.txt

This comment has been minimized.

@matklad

matklad May 29, 2018

Member

You need src[/]generated.txt here to pass the test on windows.

@matklad

This comment has been minimized.

Copy link
Member

matklad commented May 29, 2018

@bors r+

Thanks @boxdot & @gferon !

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 29, 2018

📌 Commit 987ce87 has been approved by matklad

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 29, 2018

⌛️ Testing commit 987ce87 with merge f35302c...

bors added a commit that referenced this pull request May 29, 2018

Auto merge of #5584 - boxdot:master, r=matklad
Verify that src dir wasn't modified by build.rs when publishing

Fixes issue #5073.

Implemented during RustFest Paris `impl days`.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 29, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing f35302c to master...

@bors bors merged commit 987ce87 into rust-lang:master May 29, 2018

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

@matklad matklad referenced this pull request Jun 2, 2018

Merged

Generate files in OUT_DIR #353

@vityafx

This comment has been minimized.

Copy link

vityafx commented Aug 2, 2018

Can src directory contain immutable code which includes contents of something from build dir written by build.rs? vergen crate, for example.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 2, 2018

@vityafx of course yeah!

@lu-zero

This comment has been minimized.

Copy link

lu-zero commented Aug 2, 2018

How does that mix with bindgen? I want to generate the bindings using build.rs and they usually live in src/.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Aug 2, 2018

@lu-zero build scripts should place the files they create in the OUT_DIR: https://doc.rust-lang.org/cargo/reference/build-scripts.html#case-study-code-generation

@lu-zero

This comment has been minimized.

Copy link

lu-zero commented Aug 2, 2018

Looks quite bulky (and in my case quite a bit of churn).

I can live with that, but would be nicer to provide a better mean to hide it.

@lu-zero

This comment has been minimized.

Copy link

lu-zero commented Aug 2, 2018

Also, forces me to create an empty file with just that line. Since doing

mod foo {
   include!(concat!(env!("OUT_DIR"), "/foo.rs"));
}

Does not work.

@lu-zero

This comment has been minimized.

Copy link

lu-zero commented Aug 2, 2018

Also having the empty file leads to :

error: an inner attribute is not permitted in this context
 --> /Users/lu_zero/Sources/rust/rav1e/target/debug/build/rav1e-77987e5f4b9641e6/out/aom.rs:3:3
@lu-zero

This comment has been minimized.

Copy link

lu-zero commented Aug 2, 2018

So basically you made impossible to publish crates that rely on bindgen or there is a way around it?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Aug 2, 2018

@lu-zero try using setup which is described in bindgen’s docs:

https://rust-lang-nursery.github.io/rust-bindgen/tutorial-3.html
https://rust-lang-nursery.github.io/rust-bindgen/tutorial-4.html

There’s also —no-verify flag which you can use to suppress this check. I would advise against using it, if possible: mutating sources during the build will make lives of consumers of your library harder.

@lu-zero

This comment has been minimized.

Copy link

lu-zero commented Aug 2, 2018

What is suggested does not seem to work at least with the bindgen 0.37.4.

#![allow(non_upper_case_globals)]
#![allow(non_camel_case_types)]
#![allow(non_snake_case)]

include!(concat!(env!("OUT_DIR"), "/aom.rs"));

The generated file still contains:

/* automatically generated by rust-bindgen */

#![allow(dead_code)]
#![allow(non_camel_case_types)]
#![allow(non_snake_case)]
#![allow(non_upper_case_globals)]

...

The result is :

error: an inner attribute is not permitted in this context
 --> /Users/lu_zero/Sources/rust/rav1e/target/debug/build/rav1e-22d2ecf7978d906c/out/aom.rs:3:3
  |
3 | #![allow(dead_code)]
  |   ^
  |
  = note: inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files. Outer attributes, like `#[test]`, annotate the item following them.
@lu-zero

This comment has been minimized.

Copy link

lu-zero commented Aug 2, 2018

Oh, right I forgot to change the build.rs recipe to not append those there.

haraldh added a commit to varlink/rust that referenced this pull request Aug 3, 2018

varlink-cli: build to OUT_DIR
With Rust 1.28.0:

Cargo will now no longer allow you to publish crates with build scripts
that modify the src directory.
The src directory in a crate should be considered to be immutable.

rust-lang/cargo#5584

hcpl added a commit to hcpl/mtproto-rs that referenced this pull request Sep 28, 2018

Move generating schema definitions to output dir
This is due to rust-lang/cargo#5584 that forbids
writing to `src/` and is present since Rust 1.28.

ysimonson added a commit to indradb/indradb that referenced this pull request Oct 28, 2018

ysimonson added a commit to indradb/indradb that referenced this pull request Oct 28, 2018

Revert "Try removing ignore of autogenerated files to circumvent erro…
…r caused by `cargo package` (see rust-lang/cargo#5584)"

This reverts commit 712c54a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment