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

Implement infrastructure for the souped-up build command #792

Merged
merged 28 commits into from
Nov 5, 2014

Conversation

alexcrichton
Copy link
Member

This series of commits (based on #763) is an implementation of the recent Cargo RFC. 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:

I haven't quite gotten around to curl just yet, but it's next on my list!

@alexcrichton
Copy link
Member Author

I've now added some hopefully extensive documentation about build scripts.

@@ -16,7 +16,8 @@ pub struct Manifest {
targets: Vec<Target>,
target_dir: Path,
doc_dir: Path,
build: Vec<String>,
build: Vec<String>, // TODO: deprecated, remove
Copy link
Member

Choose a reason for hiding this comment

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

do the stability markers not work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now these are actually just markers of what to remove once we remove the old system of build cmd, not necessarily for deprecated functionality itself.

@steveklabnik
Copy link
Member

@alexcrichton doc review conducted as asked ❤️ Looks really great, just a couple of nits

* `OUT_DIR` - the folder in which all output should be placed. This folder is
inside the build directory for the package being built, and it is
unique for the package in question.
* `TARGET` - the target triple that is being compiled for. Native code should be
Copy link
Member

Choose a reason for hiding this comment

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

(Side note: do we have any docs anywhere on target triples?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, not that I know of actually. That is... a bit sad though. I'll see if I can find some internet docs to link to.

@alexcrichton alexcrichton force-pushed the build-cmd branch 2 times, most recently from a4b5fb1 to ce7c253 Compare November 4, 2014 07:22
@alexcrichton
Copy link
Member Author

Updated with a re-worded code generation example and with lots of nits fixed (thanks @tomaka, @steveklabnik, and @huonw!)

I agree @steveklabnik that the last section is lacking. I think I ran out of steam once I reached that point, so I think I'll wait until morning to update it :)

@alexcrichton
Copy link
Member Author

Updated with an expanded "linking to system libraries" section. I'm still not super thrilled about it as I feel like it's just too overwhelming to show tons of code, but it's not super useful without a couple of code examples. It's at the end though and it's a case study so maybe it's not too bad?

@alexcrichton alexcrichton force-pushed the build-cmd branch 2 times, most recently from d259798 to c05442a Compare November 5, 2014 19:40
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!
@bors bors merged commit 5e29a8b into rust-lang:master Nov 5, 2014
@alexcrichton alexcrichton deleted the build-cmd branch November 5, 2014 21:30
@@ -23,6 +23,7 @@ pub struct Compilation {
///
/// This is currently used to drive some entries which are added to the
/// LD_LIBRARY_PATH as appropriate.
// TODO: deprecated, remove
Copy link
Member

Choose a reason for hiding this comment

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

Is this TODO correct, @alexcrichton? I.e. should I PR the removal of Compilation.native_dirs, or PR the removal of the TODO?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh dear that's a good question, this PR is... quite old though! I vaguely remember placing this on a few things throughout Cargo I've wished I deleted at the time and then never got to, but removing this would be good if it's not actually used in Cargo today! (or if it's actually used just remove the comment)

bors added a commit that referenced this pull request Oct 24, 2018
…=dwijnand

Remove the remote TODO on Compilation.native_dirs

It's still used, see tests:

* build_script::doctest_receives_build_link_args
* build_script::rename_with_link_search_path
* run::run_with_library_paths
* run::library_paths_sorted_alphabetically

Originally I was trying to action #792 (comment) with this PR
@ehuss ehuss mentioned this pull request Aug 4, 2019
bors added a commit that referenced this pull request Aug 5, 2019
Fix an old test.

This test was added in f888b4b, part of #792, in a commented state. Might as well make it work. It is a bit surprising that passing `-l nonexistinglib` doesn't cause an error, but seems to be fine.
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.

6 participants