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

Target-specific features reimplemented #1197 #7182

Closed
wants to merge 5,967 commits into from
Closed

Target-specific features reimplemented #1197 #7182

wants to merge 5,967 commits into from

Conversation

plyhun
Copy link

@plyhun plyhun commented Jul 26, 2019

ehuss and others added 30 commits May 20, 2019 15:22
This commit imports the external [alexcrichton/cargo-vendor
repository][repo] into Cargo itself. This means it will no longer be
necessary to install the `cargo-vendor` subcommand in order to vendor
dependencies. Additionally it'll always support the latest feature set
of Cargo as it'll be built into Cargo!

All tests were imported as part of this commit, but not all features
were imported. Some flags have been left out that were added later in
the lifetime of `cargo vendor` which seem like they're more questionable
to stabilize. I'm hoping that they can have separate PRs adding their
implementation here, and we can make a decision of their stabilization
at a later date.

The current man page for `cargo vendor -h` will look like:

    cargo-vendor
    Vendor all dependencies for a project locally

    USAGE:
	cargo vendor [OPTIONS] [--] [path]

    OPTIONS:
	-q, --quiet                    No output printed to stdout
	    --manifest-path <PATH>     Path to Cargo.toml
	    --no-delete                Don't delete older crates in the vendor directory
	-s, --sync <TOML>...           Additional `Cargo.toml` to sync and vendor
	    --respect-source-config    Respect `[source]` config in `.cargo/config`
	-v, --verbose                  Use verbose output (-vv very verbose/build.rs output)
	    --color <WHEN>             Coloring: auto, always, never
	    --frozen                   Require Cargo.lock and cache are up to date
	    --locked                   Require Cargo.lock is up to date
	-Z <FLAG>...                   Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details
	-h, --help                     Prints help information

    ARGS:
	<path>    Where to vendor crates (`vendor` by default)

    This cargo subcommand will vendor all crates.io and git dependencies for a
    project into the specified directory at `<path>`. After this command completes
    the vendor directory specified by `<path>` will contain all remote sources from
    dependencies specified. Additionally manifest beyond the default one can be
    specified with the `-s` option.

    The `cargo vendor` command will also print out the configuration necessary
    to use the vendored sources, which when needed is then encoded into
    `.cargo/config`.

Since this change is not importing 100% of the functionality of the
existing `cargo vendor` this change does run a risk of being a breaking
change for any folks using such functionality. Executing `cargo vendor`
will favor the built-in command rather than an external subcommand,
causing unimplemented features to become errors about flag usage.

[repo]: https://github.com/alexcrichton/cargo-vendor
Add message caching.

The `cache-messages` feature causes Cargo to cache the messages generated by
the compiler. This is primarily useful if a crate compiles successfully with
warnings. Previously, re-running Cargo would not display any output. With the
`cache-messages` feature, it will quickly redisplay the previous warnings.

```
cargo +nightly check -Z cache-messages
```

Notes:
- `short` messages do not work correctly.
- rustdoc does not support `--json-rendered=termcolor`, so its output is currently uncolored.
- This approach to rendering should address some output issues like #6848.
cargo package: detect new empty directories

Detect the creation of directories in a build script, which is currently a very tempting workaround for the fact that a crate built from a package will be missing any empty directories.

Maybe it would be better to only include directories in the map of hashes if they are completely empty.

The removal of a directory that is initially empty cannot be tested because, as stated above, a crate built from a package will not *have* any empty directories.

Closes #6931.
zsh: Add --all-targets option to cargo-check and cargo-build

r? @ehuss
ehuss and others added 2 commits July 26, 2019 09:06
Fix some issues with absolute paths in dep-info files.

There were some issues with how #7030 was handling translating paths in dep-info files. The main consequence is that when coupled with rust-lang/rust#61727 (which has not yet merged), the fingerprint would fail and be considered dirty when it should be fresh.

It was joining [`target_root`](https://github.com/rust-lang/cargo/blame/1140c527c4c3b3e2533b9771d67f88509ef7fc16/src/cargo/core/compiler/fingerprint.rs#L1352-L1360) which had 3 different values, but stripping [only one](https://github.com/rust-lang/cargo/blame/1140c527c4c3b3e2533b9771d67f88509ef7fc16/src/cargo/core/compiler/mod.rs#L323). This means for different scenarios (like using `--target`), it was creating incorrect paths in some cases. For example a target having a proc-macro dependency which would be in the host directory.

The solution here is to always use CARGO_TARGET_DIR as the base that all relative paths are checked against. This should handle all host/target differences.

The tests are marked with `#[ignore]` because 61727 has not yet merged.

This includes a second commit (which I can open as a separate PR if needed) which is an alternate solution to #7034. It adds dep-info tracking for registry dependencies. However, it tries to limit which kinds of paths it tracks. It will not track package-relative paths (like source files). It also adds an mtime cache to significantly reduce the number of stat calls (because every dependency was stating every file in sysroot).

Finally, I've run some tests using this PR with 61727 in rust-lang/rust. I can confirm that a second build is fresh (it doesn't erroneously rebuild). I can also confirm that the problem in rust-lang/rust#59105 has *finally* been fixed!

My confidence in all this is pretty low, but I've been unable to think of any specific ways to make it fail. If anyone has ideas on how to better test this, let me know. Also, feel free to ask questions since I've been thinking about this a lot for the past few weeks, and there is quite a bit of subtle stuff going on.
@bors
Copy link
Contributor

bors commented Jul 26, 2019

☔ The latest upstream changes (presumably #7137) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss and others added 3 commits July 26, 2019 12:53
Clean up TargetInfo

- The main motivation here is to provide a better error message when collecting information from rustc fails (it now shows the command and the output).
- Remove `has_cfg_and_sysroot`. I think this dates back to when it was introduced in #2328, as a guard for older versions of rustc that did not know about `--print=cfg`. Unless I'm missing something, I don't think we need to retain this backwards compatibility.
- Add some documentation.
- Demote the rustc cache log messages to `debug` level. I haven't really seen any caching issues, so I don't think it needs to be info level.
- Some other misc cleanup (remove unused function, etc.).
@Eh2406
Copy link
Contributor

Eh2406 commented Jul 28, 2019

Thanks for the submission! I don't have time to give this the review it deserves. I am sorry.
Currently the lockfile that is generated is completely independent of the system it is built on, does this PR maintain that property?
The metadata output is stable so we need to be careful to include the new info in a way that doesn't break anyone.

(cc #1197)

@plyhun
Copy link
Author

plyhun commented Jul 28, 2019

I was relying on the unit tests to keep the existing functionality alive, did not pay a specific attention to the lock files (though no new information should be added to the lockfile, as far as I have inspected several of the live projects that I need the platform features for). Perhaps if you find a need to check the lockfile stability, I may try implementing it as a unit test.
In principle, my change replaces the str/String/InternedString that replesents a feature, with a tuple (str/String/InternedString, Option<Platform>), and Set<String> with Map<String, Option<Platform>>, for the places where the parser interacts with the input Cargo.toml and the platform triple.

snuk182 and others added 10 commits July 28, 2019 15:39
# Conflicts:
#	src/cargo/core/dependency.rs
Update `cargo_compile` module doc.

Update a hilariously outdated comment for `ops::cargo_compile`.
Refactor resolve `Method`

This changes the enum `Method` to a struct called `ResolveOpts`. The intent is to try to make it a little easier to read and understand.

If this doesn't actually look better to anyone, I'm fine with discarding this (I can resubmit just with documentation). It only seems marginally better, but I have had difficulty with understanding the subtleties of `Method` for a while. I wasn't able to measure any performance difference.

This also has a few other changes:
- Adds some documentation.
- Removes `resolve_ws_precisely`, cutting down the number of resolve functions from 4 to 3.
@bors
Copy link
Contributor

bors commented Jul 29, 2019

☔ The latest upstream changes (presumably #7186) made this pull request unmergeable. Please resolve the merge conflicts.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 29, 2019

The tricky case I see is
root depends on a and c="1.1.0"
a depends on b and on OSX depends on b's flag maybe
b depends on c="=1.0.0" if maybe is active.
c v1.0.0 and c v1.1.0 has no dependency

On OSX we get an error "can not have two semver compatible versions of c". Do we get that error on Windows?

@plyhun
Copy link
Author

plyhun commented Jul 30, 2019

Something utterly terrible had happened during the upstream merge. Have initiated a new PR -> #7190 .

@plyhun plyhun closed this Jul 30, 2019
@plyhun plyhun deleted the target-features branch July 30, 2019 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.