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

[New custom build] Implement the new custom build command #763

Closed
wants to merge 11 commits into from

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Oct 27, 2014

Does include:

  • The build script is compiled in target/build/$pkg, its output is written in the same directory in the output file, and it has target/build/$pkg/out available to write temporary files
  • The output files are later loaded by each rustc invocation and other custom build command, in order to retreive the rustc-flags or DEP_ variables.
  • Internal rename from plugin to for_host
  • A different system for passing command descriptions to the shell (internal change)
  • Deprecation of the old custom build script system with a warning if it is being used

Does not include:

  • Support for links, I plan to add this after this PR is merged
  • DEP_<links>_<key>: You just need to uncomment a block of code, but this can't be implemented without support for links
  • Build dependencies, I plan to add this after this PR is merged ; this probably conflicts a lot with [New custom build] Implement platform-specific dependencies #735, if I implement this now, I'll need to rewrite half of the code when rebasing
  • Tests to check that rustc-flags are correctly passed to rustc, because rustc doesn't support the -l flag yet. I'll add this to the PR once rust supports it (or maybe even before if I compile the fork of rustc that has it).
  • The test supposed to check whether a custom build script failure triggers a compilation failure for the whole project is commented out, because I didn't find a way to get the name of the package to match the exact error message. This will need to be fixed before merging.
  • There's no feedback of the new system in the Compilation struct
  • The build directory is not used when determining the fingerprint, something that was apparently done for the native directory

I recommend reviewing commit by commit. I implemented each feature one by one (tests should pass for each individual commit).

cc #610

@tomaka tomaka changed the title Implement the new custom build commands [New custom build] Implement the new custom build command Oct 27, 2014
}

/// Consumes this job by running it, returning the result of the
/// computation.
pub fn run(self, fresh: Freshness) -> CargoResult<()> {
pub fn run(self, fresh: Freshness, sender: Sender<String>) -> CargoResult<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/sender/tx/

@alexcrichton
Copy link
Member

Ok, I've looked this over and added some comments here and there, thanks @tomaka!

Could you also add some tests along these lines:

  • build commands and cargo {test,run,bench,doc}
  • propagating of -L throughout the entire dependency graph when one package requests it
  • If I change a file in a package with a build command the build command is re-run

The list of things you plan on adding after this gets merged looks great, I hope to merge this soon! Some specific comments:

There's no feedback of the new system in the Compilation struct

This is currently mainly used for feeding information into cargo {test,bench,run} which uses the output of compilation to actually run some commands. I don't think we'll need to modify them much, but you'll need to be sure that any -L paths are indeed included in LD_LIBRARY_PATH.

The build directory is not used when determining the fingerprint, something that was apparently done for the native directory

Could you elaborate a little more on this?

@alexcrichton
Copy link
Member

Ah, and one other recommendation for tests: Could you pass -v wherever possible and match the entire output of all commands? I'd just want to make sure that build commands are printed as being run wherever possible.

@tomaka
Copy link
Contributor Author

tomaka commented Oct 27, 2014

Could you elaborate a little more on this?

I re-read Cargo's source code, and apparently I was mistaken. When I modified Layout, I noticed several references to native() in fingerprint.rs, but on a second look it's just here to fill the Compilation struct.

@alexcrichton
Copy link
Member

Ok, sounds good to me!

@tomaka tomaka force-pushed the build-cmd branch 2 times, most recently from 3223825 to c1ed49c Compare October 29, 2014 10:16
@tomaka
Copy link
Contributor Author

tomaka commented Oct 29, 2014

For the record, here's what remains to be done before merging:

  • Extract code in cargo_rustc::compile to its own function
  • Add tests for build commands and cargo {test,run,bench,doc}, don't forget the -v flag
  • Add test for "If I change a file in a package with a build command the build command is re-run"
  • Add tests for -l and -L flags being passed correctly (including -L between dependencies)

@tomaka
Copy link
Contributor Author

tomaka commented Nov 2, 2014

Closing in favor of #792

@tomaka tomaka closed this Nov 2, 2014
bors added a commit that referenced this pull request Nov 5, 2014
This series of commits (based on #763) is an implementation of the recent [Cargo RFC](https://github.com/rust-lang/rfcs/blob/master/text/0403-cargo-build-command.md). This should implement all portions of the RFC, but there's a lot so an extra set of eyes would be nice!

I haven't added documentation for it all yet, but I would like to do so before landing (starting with #749). Otherwise I've been migrating all of the existing cargo dependencies away from the build command to a build script, and the progress can be seen with these repositories:

* https://github.com/alexcrichton/gcc-rs
* https://github.com/alexcrichton/pkg-config-rs
* https://github.com/alexcrichton/git2-rs/tree/build-cmd
* https://github.com/alexcrichton/openssl-sys
* https://github.com/alexcrichton/flate2-rs/tree/build-cmd
* https://github.com/alexcrichton/libz-sys
* https://github.com/alexcrichton/ssh2-rs/tree/build-cmd

I haven't quite gotten around to curl just yet, but it's next on my list!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants