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 "artifact dependencies" (RFC-3028) #9992

merged 1 commit into from Feb 22, 2022


Copy link

@Byron Byron commented Oct 21, 2021

Tracking issue: #9096


  • -Z unstable option called bindeps
  • (config) allow parsing of newly introduced 'artifact' fields
    • into TomlManifest
    • into Manifest
      • abort warn if artifacts are used on stable
  • resolver : assure artifact dependencies are part of the resolution process and unified into the dependency tree
  • 🔬compiler: make it understand 'artifact' dependencies and pass new environment variables to the crate being build
    • lib=false should not be considered a rust library for the dependent, in unit and possibly resolve graph
    • assure profile settings are applied correctly
    • target overrides work
      • target = "target" in build deps
      • other targets on build deps
      • other targets on non-build deps
      • 'no-cross doc tests' seems like a constraint we should apply as well maybe
    • more confidence with resolver = "2"
    • assure artifact placement is correct (bin and various forms of lib)
  • serialization: rewriting manifests (i.e. for publishing) does not discard artifact information
    • publishing keeps artifact and lib values
  • Other cargo subcommands
    • cargo metadata
      • leave unchanged
    • artifacts work with cargo check
    • artifacts work with rustdoc, such that it doesn't document them unless lib=true
    • cargo tree maybe?
    • cargo clean should clean artifacts - even though it's more complex, ultimately it deletes the target directory.
    • artifacts work with cargo test (and dev-dependencies)
      • doctests
      • try reproducible repository as well.
  • 🧪 tests for more subtle RFC constraints
    • build scripts cannot access artifact environment variables at compile time, only at runtime)
    • assure 'examples' which also support crate-type fields like [[lib]] won't become artifacts themselves.
    • assure --out-dir does not leak artifacts - tested manually, it seemed to niche to add a test.
    • try target="foo" in artifact and assure it sees a decent error message
    • Assure RFC 3176 doesn't work
  • 🧹cleanup and finalization
    • assure no TODO(ST) markers are left in code
    • assure no tests are ignored
    • use resolver = "1" once to assert everything also works with the previous resolver, but leave it on "2".

Implementation and review notes

  • artifacts and unstable options are only checked when transforming them from TomlManifest to Manifest, discarding artifact information if the unstable flag is not set. Nowhere else in code will the CLI options be checked again.
  • If no binaries are specified, all the binaries in the package will be built and made available. - this should only refer to [[bin]] targets, not examples for instance.
  • artifact binaries won't be uplifted, hence won't be present outside of their output directory
  • ❗️We don't know how package links will affect artifacts for build dependencies. Should probably be thought through.
  • ❗️The location of artifacts is only tested roughly to avoid having to deal with different output names on the four platforms that seem to matter (gnu, macos, windows msvc, windows gnu).
  • cargo tree doesn't handle artifacts specifically, and it might be interesting to make clear if an artifact is only an artifact, or both artifact and dependency.
  • Most error and warning messages can probably be more cargo-matic.


  • Does cargo without the feature enabled have to complain about the "artifact" field in a dependency, like it does right now? It doesn't look like machinery for that exists in do_read_manifest().
    • ✔️It warns now
  • Should parsing of artifact values, like "bin" be case sensitive?
    • ✔️ It's case sensitive now, which should help with serde roundtripping.

Review Progress

  • address Josh's review notes one by one
    • introduce IsArtifact (see this answer) (76cd48a2d62d74e043a1a482199c5bb920f50311)
    • prefer uplifting artifact deps that were written into the deps directory, but prefer to do that in this PR instead
    • add extra-tests as described in Josh's comment: " features get unified between a Rust library and a binary, and one that confirms features don't get unified between a Rust library and a binary for a different target?"
    • Make target-based artifact splitting work by porting parts of RFC-3176
      • test-support/cross-compile
      • namespace separation
  • re-create RFC-3176 what's not in RFC-3028, namely multidep support and all related tests
  • Address Eh2406 's review comments
  • Address Eric's comments
    • Unstable features need to be documented in
    • sort out target_data
    • figure out cargo metadata
    • See if target-data can work without an index format update.
    • truncate comments at 80-90 lines and remove unused methods, remove -Z unstable-options, use cfg!(target_env = "msvc")
    • add missing doc comments to newly added methods and funtions
    • simplify method parameters and inline some functions
    • add test and extend test to show additional issues
  • assure current set of tests works consistently, also on windows

Sponsored by Profian

Copy link

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2021
@Byron Byron marked this pull request as draft October 22, 2021 09:40
@Byron Byron changed the title [DRAFT] Implement "artifact dependencies" (RFC-3028) Implement "artifact dependencies" (RFC-3028) Oct 22, 2021
@Byron Byron force-pushed the rfc-3028 branch 3 times, most recently from fde870a to 4159ead Compare October 23, 2021 09:42
Copy link

This is looking really exciting, thanks for putting it together!!

One thing I noticed when experimenting with it is that these deps don't currently appear in the resolve graph for cargo-metadata output because of this filtering in the case that an artifact dep doesn't happen to have a lib target - it would be lovely if they did, perhaps with some kind of kind specifier in the output. Before I knew this work was going to be picked up, I recently put together #9974 which I'm very happy to adapt to this PR, or to point to as a source for a test-case if you'd rather pick up that piece :)

Copy link
Member Author

Byron commented Oct 23, 2021

This is looking really exciting, thanks for putting it together!!

So good to see a fellow here :)! Thus far I am trying to understand what has to go where, but have a feeling I am getting closer the better I understand how all this works. The most difficult part would be to not end up with a hack, but implement it as it would be intended 😅.

One thing I noticed when experimenting with it is that these deps don't currently appear in the resolve graph for cargo-metadata output because of this filtering in the case that an artifact dep doesn't happen to have a lib target - it would be lovely if they did, perhaps with some kind of kind specifier in the output. Before I knew this work was going to be picked up, I recently put together #9974 which I'm very happy to adapt to this PR, or to point to as a source for a test-case if you'd rather pick up that piece :)

Thanks for the pointer! Right now the artifact information is merely present in a non-conflicting way until I am sure where it would natively belong. Right now cargo considers artifact dependencies libraries as per crate-type, which is probably wrong, and I am missing tests to trigger this (changing that shouldn't be the problem though). My plan now was to look in reverse after ending up staring at the UnitGraph for a while, and work backwards from the place where environment variables are set for build during execution and for rustc invocation. Maybe that way it becomes clear how artifact dependencies should be represented in the type system.

I'd love to implement artifact dependencies so that cargo behaves 'as expected' no matter which sub-command is used, but I don't know if that has to mean that artifact dependencies are always considered libs in the sense of is_lib() == true. Probably they are their own type and if lib = true in the manifest, they are indeed considered libraries. Whether or not the filtering done by cargo-metadata is desirable is then probably best solved independently.

All this I say from a place of utter ignorance and am easily swayed to do 'the right thing'™️ if somebody knows what that would be.

Copy link
Member Author

Byron commented Oct 24, 2021


This comment describes the progress so far, along with an idea on how it's going, roughly based on the continuously updated comment in this PR to represent the current state of affairs.

On the bright side, it was quite straightforward to add a feature toggle to cargo and allow schema-validated artifact information to the manifest format. Writing tests for everything, both happy and unhappy paths, is very powerful run the whole machinery and make assertions on the output. Along the more than 2200 tests present today there is always one example I could use to test even the most intricate detail. For now, tests validating the actual functionality described in the RFC are quite basic and mostly ignored as they aren't passing yet - no actual implementation was possible yet.

After trying and discarding the idea to start in reverse and see where environment variables are set, following the flow of data backwards, I settled for making target validation tests pass first. These tests should fail if an artifact dependency declares a cdylib, for instance, but refers to a crate that doesn't actually have a [[lib]] of such type. Error messages related to failed feature resolution brought me to the resolver, which unfortunately doesn't yet have the required target information (contained in Package) available. Thus it looks like I have to continue going to the Unit level which has Packages available. From there, I hope, it will become clearer where to put the logic to handle Artifacts properly.

Overall, I am not terribly happy with the progress thus far but would hope that once the two 'right' spots to add artifact logic have been found, development can speed up and get into the usual 'test + impl' cycle.

Copy link
Member Author

Byron commented Oct 31, 2021

Finally, a breakthrough, and the key for that was something I didn't use in what feels like a decade: a debugger. Looking at the complex cargo code while imagining what might be in there at runtime based on some manifests can only get you so and so far, and was seriously reducing my progress.

Everything cleared up quickly after the first few debugging sessions, and seeing actual data being handled by the code is just changed the game.

Hence, build scripts are now able to receive artifact dependencies at runtime, which are also placed in the correct directories. Thus far I appear to have been minimally invasive, too, which might indicate the changes where made in a not too far off place.

Next steps are making artifact environment variables available at build time and writing many more tests to validate more subtle behaviors, and I don't think there is going to be more roadblocks on the way am optimistic it will be smooth sailing from here on.

Copy link

@Byron Congratulations! Does this work for cross-target compiles too? Or is this just homogeneous targets so far?

Copy link
Member Author

Byron commented Oct 31, 2021

🎉It's just homogenous thus far, and there aren't even tests yet to see how different target types interact. However, from what I see the target can freely be specified so I expect no resistance there.

Copy link

r? @joshtriplett

Copy link
Member Author

Byron commented Nov 3, 2021

@joshtriplett I am now at a point where artifact deps are created both for build script runs as well as for libraries. However, the limitation of what looks like a race condition is reproducible on my machine at all times, meaning that the binary artifact isn't actually present (yet) when the build is executed, causing include_bytes!(env!(<ARTIFACT>)) to fail even though the file is present right after (and will become available a little later as build scripts are able to wait for it).

Oddly enough, the last time I checked everything seemed to have run exactly as expected, and I concluded it's impossible for Cargo to start a job if its dependencies aren't yet ready. Maybe, just maybe, it's related to rmeta though which is able to kick-off the dependents before the final artifact is produced, but a quick look didn't yield any evidence for that.

Maybe it's possible that edges are removed too eagerly which allows starting jobs too early. Will look into this a little more as it appears to be a lead.

edit: fixed 🎉

Copy link
Member Author

Byron commented Nov 4, 2021

It looks better by the day and now build-scripts at runtime as well as libs at build time have access to artifacts as described in the RFC. Besides plenty of missing tests for edge-cases the big topics to look at next is to get targets and profile settings right. I don't expect issues there and hope it will be as straightforward as I can predict based on my current knowledge of the inner workings.

Copy link

Hezuikn commented Nov 5, 2021

Copy link
Member Author

Byron commented Nov 5, 2021

@Hezuikn I think the best way to post questions/issues during development is by providing a cargo test, using its excellent test framework, to reproduce the issue. From what I can tell in the linked repository is that the environment variable used in isn't anything that would be set by artifact dependencies.

Copy link

Hezuikn commented Nov 5, 2021

the forwards the artifact env var to the cargo test of the root crate; everything works as expected but only when any of the two lines in ./Cargo.toml are uncommented when* they shouldnt be needed

Copy link

Hezuikn commented Nov 5, 2021

i also forgot to say that it gives me ICE: [hezuikn@harch reproduceable]$ cargo test
thread 'main' panicked at 'activated_features for invalid package: features did not find PackageId { name: "test", version: "0.1.0", source: "/home/hezuikn/reproduceable/test" } false', src/cargo/core/resolver/
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

Copy link
Member Author

Byron commented Nov 5, 2021

Now I see! I will be sure to add cargo test tests and see if both issues can be made reproducible in the test suite.

Copy link
Member Author

Byron commented Nov 5, 2021

@Hezuikn Maybe you could try the test again with the latest version of this - I now have a test that runs cargo test and fixed a shortcoming that should affect it. Please note that doctest doesn't work yet.

Copy link

Hezuikn commented Nov 5, 2021

cargo check, cargo build, cargo test all still ice with the same err; ive further minimized reproducible

Copy link
Member Author

Byron commented Nov 6, 2021

@Hezuikn I have added the setup from reproducible to the test suite and fixed the issue with resolver = "2". resolver = "1" works because it handles build features differently, and adding another dependency to the same package that isn't a build dependency will put just the right entry into the map that is queried there, preventing it to bail out.

Also, is it common to call a cargo error ICE, which seems to be an internal compiler error? I am asking because I would think these would be panics if a precondition isn't satisfied, and in this case I am not sure if recovery is possible. Hence my fix is to 'give it the flag that it wants' because it seems the right thing to do judging from the flags documentation, yet I am not quite sure if the code should bail in the first place or have that desired entry in the map. 6584917 shows the fix in case you have deeper insight into this.

Copy link

Hezuikn commented Nov 6, 2021

no thats just me; reproduceable has new breakage!

Copy link
Member Author

Byron commented Feb 18, 2022

After rebasing against master to fix CI I did my best to update all comments with the rewritten commit IDs. Even though I did my best, in case there is a mistake I leave the commit mapping here which should allow to sort things out with the help of looking at older versions of commit messages.

  • 78f0add -> 78213ea
  • f84309f79be6d089faa4437f06aaeae22cf4fcdd -> 3de5799
  • 61b285cdd4a2ff45a5d830b3a95f6496f9fa4601 -> b478370
  • c0e6abe384c2c6282bdd631e2f2a3b092043e6c6 -> 0cf581e
  • 9ebc4e156f0efa60751ae2199cd70dc71b90f769 -> 25218d2
  • 64aa29e61b809db9a84c876d56a5a9a0da9fb3000 -> cd25256

Copy link

ehuss commented Feb 18, 2022

Regarding the filtering, the following diff is roughly what I was thinking: ehuss@41d85cc

Essentially, that keeps state.deps() as a single way to fetch dependencies, and all filtering is the same in all scenarios. It also tries to simplify the interface with calc_artifact_deps.

I think that cuts about 40 lines of code, and makes the filtering of deps a little more straightforward. What do you think?

@bstrie bstrie mentioned this pull request Feb 18, 2022
12 tasks
Copy link
Member Author

Byron commented Feb 21, 2022

Thanks so much, @ehuss, for the all the help!

I think that cuts about 40 lines of code, and makes the filtering of deps a little more straightforward. What do you think?

I think it's something I would have loved to come up with the first time around, and believe it's a great improvement. I couldn't find anything to add or change so pushed the commit into this PR as is.


Due to recent events I feel the need to once again commend the reviewers and @ehuss in particular for their amazing communication skills when reviewing PRs like this. I can only imagine how much work it means and how silly some of the changes proposed here might look to a seasoned cargo developer, yet you maintain a constructive, upbeat, and friendly spirit at all times. It's a style that I am aspiring when reviewing PRs myself, and is a prime example for the accessibility and friendliness of the Rust community as a whole.

Thank you!


Copy link

ehuss commented Feb 21, 2022

Okay, I think this is probably ready to go. Can you squash the commits? I think it would be fine to just have one commit, but I'll leave it up to you if you want to keep a few extras.

Tracking issue: rust-lang#9096
Original PR: rust-lang#9992

Add 'bindeps' -Z flag for later use

A test to validate artifact dependencies aren't currently parsed.

Parse 'artifact' and 'lib' fields.

Note that this isn't behind a feature toggle so 'unused' messages will

Transfer artifact dependencies from toml- into manifest-dependencies

There are a few premises governing the operation.

- if unstable features are not set, warn when 'artifact' or 'lib' is
- bail if 'lib' is encountered alone, but warn that this WOULD happen
  with nightly.
- artifact parsing checks for all invariants, but some aren't tested.

Assure serialization of 'artifact' and 'lib' fields produces suitable values during publishing

This should be the only place were these fields matter and where a cargo
manifest is actually produced. These are only for internal use, no user
is typically going to see or edit them.

Place all artifact dependency tests inta their own module

This facilitates deduplication later and possibly redistribution into
other modules if there is a better fit.

Represent artifacts that are rust libraries as another ArtifactKind

This is more consistent and probably simpler for later use.
No need to reflect the TOML data structure.

Add tests to assure only 'lib = true' artifact deps are documented

RFC-3028 doesn't talk about documentation, but for lib=true it's clear
what the desired behaviour should be.
If an artifact isn't a library though, then for now, it's transparent,

Many more tests, more documentation, mild `Artifact` refactor

The latter seems to be a better fit for what being an artifact
really means within cargo, as it literally turns being a library
on or off, and thus only optionally becoming a normal library.

refactor to prepare for artifact related checks

Don't show a no-lib warning for artifact dependencies (with lib = false)

Tests for more artifact dependency invariants

These are merely a proof of concept to show that we are not in
a position to actually figure out everything about artifacts
right after resolution.

However, the error message looks more like a fatal error and less
like something that can happen with a more elaborate error message
with causes.

This might show that these kind of checks might be better done later
right before trying to use the information for create compile units.

Validate that artifact deps with lib=true still trigger no-lib warnings

This triggers the same warning as before, for now without any
customization to indicate it's an artifact dependency.

Use warnings instead of errors

This avoids the kind of harsh end of compilation in favor of something
that can be recovered from. Since warnings are annoying, users will
probably avoid re-declaring artifact dependencies.

Hook in artifact dependencies into build script runs

Even though we would still have to see what happens if they have a lib
as well. Is it built twice?


- fly-by refactor: fix typo; use ? in method returning option
- Propagate artifact information into Units; put artifacts into place

  This means artifacts now have their own place in the 'artifact'
  directory and uplifts won't happen for them.

- refactor and fix cippy suggestion
- fix build after rebasing onto master

Create directories when executing the job, and not when preparing it.

also: Get CI to work on windows the easy way, for now.

Set directories for artifact dependencies in build script runtimes

Test remaining kinds of build-script runtime environment variables

- Fix windows tests, the quick way.
- Try to fix windows assertions, and generalize them
- Fix second test for windows, hopefully

test for available library dependency in build scripts with lib = true

probably generally exclude all artifact dependencies with lib=false.

Pass renamed dep names along with unit deps to allow proper artifact env names

Test for selective bin:<name> syntax, as well as binaries with dashes

Test to assure dependency names are transformed correctly

assure advertised binaries and directories are actually present

This wouldn't be the case if dependencies are not setup correctly,
for instance.

 - make it easier to see actual values even on failure

   This should help figure out why on CI something fails that works
   locally no matter what.
   Turns out this is a race condition, with my machine being on the good
   side of it so it doesn't show in testing. Fortunately it still can be
   reproduced and easily tested for.

 - refactor test; the race condition is still present though

 - Force CI to pass here by avoiding checks triggering race.

 - Fix windows build, maybe?

More tolerant is_file() checks to account for delay on CI

This _should_ help CI to test for the presence which is better than
not testing at all.

This appears to be needed as the output file isn't ready/present in time
for some reason.

The root cause of this issue is unknown, but it's definitely a race
as it rarely happens locally. When it happened, the file was always
present after the run.
Now we will learn if it is truly not present, ever, or if it's maybe
something very else.

Validate libs also don't see artifact dependencies as libraries with lib=false


 - Add prelimiary test for validating build-time artifacts
 - Try to fix CI on gnu windows

   Which apparently generates paths similar to linux, but with .exe suffix.
   The current linux patterns should match that.

 - refactor

   Help sharing code across modules

allow rustc to use artifact dep environment variables, but…

…it needs some adjustments to actually setup the unit dependency graph
with artifacts as well.
Right now it will only setup dependencies for artifacts that are libs,
but not the artifacts themselves, completely ignoring them when they
are not libs.

Make artifact dependencies available in main loop

This is the commit message #2:

rough cut of support for artifact dependencies at build time…

…which unfortunately already shows that the binary it is supposed to
include is reproducibly not ready in time even though the path is
correct and it's present right after the run.

Could it be related to rmeta?

This is the commit message rust-lang#3:

Fix test expectations as failure is typical than the warning we had before…

…and add some tolerance to existing test to avoid occasional failures.

This doesn't change the issue that it also doens't work at all for
libraries, which is nicely reproducable and hopefully helps to fix
this issue.

This is the commit message rust-lang#4:

Probably the fix for the dependency issue in the scheduler

This means that bin() targets are now properly added to the job graph
to cause proper syncing, whereas previously apparently it would
still schedule binaries, but somehow consider them rmeta and thus
start their dependents too early, leading to races.

This is the commit message rust-lang#5:

Don't accidentally include non-gnu windows tests in gnu windows.

Support cargo doc and cargo check

The major changes here are…

- always compile artifacts in build mode, as we literally want the
  build output, always, which the dependent might rely on being present.
- share code between the rather similar looking paths for rustdoc and

Make artifact messages appear more in line with cargo by using backticks

Also: Add first test for static lib support in build scripts

build-scripts with support for cdylib and staticlib

 - Fix windows msvc build

   No need to speculate why the staticlib has hashes in the name even
   though nothing else.

staticlib and cdylib support for libraries

test staticlib and cdylibs for rustdoc as well.

Also catch a seemingly untested special case/warning about the lack
of linkable items, which probably shouldn't be an issue for artifacts
as they are not linkable in the traditional sense.

more useful test for 'cargo check'

`cargo check` isn't used very consistently in tests, so when we use it
we should be sure to actually try to use an artifact based feature
to gain some coverage.

verify that multiple versions are allowed for artifact deps as well.

also: remove redundant test

This is the commit message #2:

Properly choose which dependencies take part in artifact handling

Previously it would include them very generously without considering
the compatible dependency types.

This is the commit message rust-lang#3:

a more complex test which includes dev-dependencies

It also shows that doc-tests don't yet work as rustdoc is run outside of
the system into which we integrate right now.

It should be possible to write our environment variable configuration
in terms of this 'finished compilation' though, hopefully with
most code reused.

This is the commit message rust-lang#4:

A first stab at storing artifact environment variables for packages…

…however, it seems like the key for this isn't necessarily correct
under all circumstances. Maybe it should be something more specific,
don't know.

This is the commit message rust-lang#5:

Adjust key for identifying units to Metadata

This one is actually unique and feels much better.

This is the commit message rust-lang#6:

Attempt to make use of artifact environment information…

…but fail as the metadata won't match as the doctest unit is, of course,
its separate unit. Now I wonder if its possible to find the artifact
units in question that have the metadata.

Properly use metadata to use artifact environment variables in doctests

This is the commit message #2:

Add test for resolver = "2" and build dependencies

Interestingly the 'host-features' flag must be set (as is seemingly
documented in the flags documentation as well), even though I am not
quite sure if this is the 100% correct solution. Should it rather
have an entry with this flag being false in its map? Probably not…
but I am not quite certain.

This is the commit message rust-lang#3:

set most if not all tests to use resolver = "2"

This allows to keep it working with the most recent version while
allowing to quickly test with "1" as well (which thus far was working

All tests I could imagine (excluding target and profiles) are working now

Crossplatform tests now run on architecture aarm64 as well.

More stringent negative testing

Fix incorrect handling of dependency directory computation

Previously it would just 'hack' the deps-dir to become something very
different for artifacts.

This could easily be fixed by putting the logic for artifact output
directories into the right spot.

A test for cargo-tree to indicate artifacts aren't handled specifically

Assure build-scripts can't access artifacts at build time

Actual doc-tests with access to artifact env vars

All relevant parsing of `target = [..]`

Next step is to actually take it into consideration.

A failing test for adjusting the target for build script artifacts using --target

Check for unknown artifact target triple in a place that exists for a year

The first test showing that `target="target"` deps seemingly work

For now only tested for build scripts, but it won't be much different
for non-build dependencies.

build scripts accept custom targets unconditionally

Support target setting for non-build dependencies

This is the commit message #2:

Add doc-test cross compile related test

Even though there is no artifact code specific to doc testing, it's
worth to try testing it with different target settings to validate
it still works despite doc tests having some special caseing around
target settings.

This is the commit message rust-lang#3:

A test to validate profiles work as expected for build-deps and non-build deps

No change is required to make this work and artifact dependencies 'just work'
based on the typical rules of their non-artifact counterarts.

This is the commit message rust-lang#4:

Adjust `cargo metadata` to deal with artifact dependencies

This commit was squashed and there is probably more that changed.

This is the commit message rust-lang#5:

Show bin-only artifacts in "resolve" of metadata as well.

This is the commit message rust-lang#6:

minor refactoring during research for RFC-3176

This will soon need to return multiple extern-name/dep-name pairs.

This is the commit message rust-lang#7:

See if opt-level 3 works on win-msvc in basic profile test for artifacts

This is the same value as is used in the other test of the same name,
which certainly runs on windows.

This is the commit message rust-lang#8:


Assure the type for targets reflect that they cannot be the host target,
which removes a few unreachable!() expressions.

Put `root_unit_compile_kind` into `UnitFor`

Previously that wasn't done because of the unused `all_values()`
method which has now been deleted as its not being used anyomre.

This allows for the root unit compile kind to be passed as originally
intended, instead of working around the previous lack of extendability
of UnitFor due to ::all_values().

This is also the basis for better/correct feature handling once
feature resolution can be depending on the artifact target as well,
resulting in another extension to UnitFor for that matter.


 - Fix ordering

   Previously the re-created target_mode was used due to the reordering
   in code, and who knows what kind of effects that might have
   (despite the test suite being OK with it).

   Let's put it back in place.

 - Deactivate test with filename collision on MSVC until RFC-3176 lands

Avoid clashes with binaries called 'artifact' by putting 'artifact/' into './deps/'

This commit addresses review comment rust-lang#9992 (comment)

Don't rely on operator precedence for boolean operations

Now it should be clear that no matter what the first term is,
if the unit is an artifact, we should enqueue it.

Replace boolean and `/*artifact*/ <bool>` with `IsArtifact::(Yes/No)`

fix `doc::doc_lib_false()` test

It broke due to major breakage in the way dependencies are calculated.

Now we differentiate between deps computation for docs and for building.

Avoid testing for doctest cross-compilation message

It seems to be present on my machine, but isn't on linux and it's
probably better to leave it out entirely and focus on the portions
of consecutive output that we want to see at least.

A test to validate features are unified across libraries and those in artifact deps in the same target

Allow aarch64 MacOS to crosscompile to an easily executable alternative target

That way more tests can run locally.

Support for feature resolution per target

The implementation is taken directly from RFC-3176 and notably lacks
the 'multidep' part.

Doing this definitely has the benefit of making entirely clear
'what is what' and helps to greatly reduce the scope of RFC-3176
when it's rebuilt based on the latest RF-3028, what we are implementing
right now.

- A test which prooves that artifact deps with different target don't have a feature namespace yet

- Add a test to validate features are namespaced by target

  Previously it didn't work because it relies on resolver = "2".

- 'cargo metadata' test to see how artifact-deps are presented

- Missed an opportunity for using the newly introduced `PackageFeaturesKey`

- Use a HashMap to store name->value relations for artifact environment variables

  This is semantically closer to what's intended.

  also: Remove a by now misleading comment

Prevent resolver crash if `target = "target"` is encountered in non-build dependencies

A warning was emitted before, now we also apply a fix.

Previously the test didn't fail as it accidentally used the old
resolver, which now has been removed.

Abort in parsing stage if nightly flag is not set and 'artifact' is used

There is no good reason to delay errors to a later stage when code
tries to use artifacts via environment variables which are not present.

Change wording of warning message into what's expected for an error message

remove unnecessary `Result` in `collect()` call

Improve logic to warn if dependencie are ignored due to missing libraries

The improvement here is to trigger correctly if any dependency of a
crate is potentially a library, without having an actual library target
as part of the package specification.

Due to artifact dependencies it's also possible to have a dependency
to the same crate of the same version, hence the package name
isn't necessarily a unique name anymore. Now the name of the actual
dependency in the toml file is used to alleviate this.

Various small changes for readability and consistency

A failing test to validate artifacts work in published crates as well

Originally this should have been a test to see target acquisition works
but this more pressing issue surfaced instead.

Make artifacts known to the registry data (backwards compatible)

Now artifacts are serialized into the registry on publish (at least
if this code is actually used in the real crates-io registry) which
allows the resolve stage to contain artifact information.

This seems to be in line with the idea to provide cargo with all
information it needs to do package resolution without downloading
the actual manifest.

Pick up all artifact targets into target info once resolve data is available

Even though this works in the test at hand, it clearly shows there
is a cyclic dependency between the resolve and the target data.

In theory, one would have to repeat resolution until it settles
while avoiding cycles.

Maybe there is a better way.

Add `bindeps`/artifact dependencies to `` with examples

Fix tests

Various small improvements

Greatly simplify artifact environment propagation to commands

Remove all adjustments to cargo-metadata, but leave tests

The tests are to record the status quo with the current code
when artifact dependencies are present and assure the information
is not entirely non-sensical.

Revert "Make artifacts known to the registry data (backwards compatible)"

This reverts commit adc5f8ad04840af9fd06c964cfcdffb8c30769b0.

Ideally we are able to make it work without altering the registry
storage format. This could work if information from the package
set is added to the resolve information.

Enrich resolves information with additional information from downloaded manifests

Resolve information comes from the registry, and it's only as rich as
needed to know which packages take part in the build.

Artifacts, however, don't influence dependency resolution, hence it
shouldn't be part of it.

For artifact information being present nonetheless when it matters,
we port it back to the resolve graph where it will be needed later.

Collect 'forced-target' information from non-workspace members as well

This is needed as these targets aren't present in the registry and
thus can't be picked up by traversing non-workspce members.

The mechanism used to pick up artifact targets can also be used
to pick up these targets.

Remove unnecessary adjustment of doc test

refactor `State::deps()` to have filter; re-enable accidentally disabled test

The initial rebasing started out with a separted `deps_filtered()`
method to retain the original capabilities while minimizing the chance
for surprises. It turned out that the all changes combined in this PR
make heavy use of filtering capabilities to the point where
`deps(<without filter>)` was unused. This suggested that it's required
to keep it as is without a way to inline portions of it.

For the original change that triggered this rebase, see


The fix originally made was reapplied by allowing to re-use the
required filter, but without inlining it.

Always error on invalid artifact setup, with or without enabled bindeps feature

Clarify how critical resolver code around artifact is working

Remove workaround in favor of deferring a proper implementation

See rust-lang#9992 (comment)
for reference and the TODO in the ignored test for more information.

truncate comments at 80-90c; cleanup

- remove unused method
- remove '-Z unstable-options'
- improve error message
- improve the way MSVC special cases are targetted in tests
- improve how executables are found on non MSVC

Avoid depending on output of rustc

There is cyclic dependency between rustc and cargo which makes it
impossible to adjust cargo's expectations on rustc without leaving
broken commits in rustc and cargo.

Add missing documentation

fix incorrect removal of non-artifact libs

This is also the first step towards cleaning up the filtering logic
which is still making some logic harder to understand than needs be.

The goal is to get it to be closer to what's currently on master.

Another test was added to have more safety regarding the overall
library inclusion logic.

inline `build_artifact_requirements_to_units()`

Simplify filtering

This adds a default filter to `state.deps(…)` making it similar to
what's currently in master, while creating another version of it
to allow setting a custom filter. This is needed as the default filter
won't allow build dependencies, which we need in this particular case.

`calc_artifact_deps(…)` now hard-codes the default filter which is
needed due to the use of `any` here:

Simplify filtering.
Copy link
Member Author

Byron commented Feb 22, 2022

Awesome! I have squashed everything into one commit, but will leave the original patch queue available in my fork at .

I was wondering if I did that correctly as now one would have to compare both diffs to validate they are still the same as the original and reviewed history is lost.

That seems to be the case for me.

➜  cargo git:(rfc-3028) ✗ git diff $(git merge-base rfc-3028-unsquashed master) rfc-3028-unsquashed | shasum
04a4fa88a4009e624c55d13c9894e4f9c5b5d8b3  -
➜  cargo git:(rfc-3028) ✗ git diff $(git merge-base rfc-3028 master) rfc-3028 | shasum
04a4fa88a4009e624c55d13c9894e4f9c5b5d8b3  -

Copy link

ehuss commented Feb 22, 2022

🎉 Looks good! 🎉

Yea, it would be nice if git made it easier to squash a branch and be confident it does the right thing.

I'm going to go ahead and merge. I'll follow up with some issues for followup work, and will try to provide some guidance in them. It may take time for me to cover all the things I'm thinking of, though.

@bors r+

Copy link

bors commented Feb 22, 2022

📌 Commit 7248f4b has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2022
Copy link

bors commented Feb 22, 2022

⌛ Testing commit 7248f4b with merge 5aad9b3...

Copy link

bors commented Feb 22, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 5aad9b3 to master...

@bors bors merged commit 5aad9b3 into rust-lang:master Feb 22, 2022
@rfcbot rfcbot removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Feb 22, 2022
@Byron Byron deleted the rfc-3028 branch February 22, 2022 05:33
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2022
Update cargo

8 commits in ea2a21c994ca1e4d4c49412827b3cf4dcb158b1d..d6cdde584a1f15ea086bae922e20fd27f7165431
2022-02-15 04:24:07 +0000 to 2022-02-22 19:55:51 +0000
- Add common profile validation. (rust-lang/cargo#10411)
- Add -Z check-cfg-features to enable compile-time checking of features (rust-lang/cargo#10408)
- Remove invalid target-specific dependency example. (rust-lang/cargo#10401)
- Fix errors in `cargo fetch` usage guide (rust-lang/cargo#10398)
- Fix some broken doc links. (rust-lang/cargo#10404)
- Implement "artifact dependencies" (RFC-3028) (rust-lang/cargo#9992)
- Print executable name on cargo test --no-run rust-lang/cargo#2 (rust-lang/cargo#10346)
- Avoid new deprecation warnings from clap 3.1.0 (rust-lang/cargo#10396)
@ehuss ehuss added this to the 1.61.0 milestone Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
None yet

Successfully merging this pull request may close these issues.

None yet