Metabuild (RFC 2196) #5628

ehuss · 2018-06-11T20:55:58Z

No description provided.

rust-highfive · 2018-06-11T20:56:08Z

(rust_highfive has picked a reviewer for you, use r? to override)

ehuss · 2018-06-11T20:57:21Z

There is a major hack/problem with the current implementation and I wanted to see if anyone had ideas on how to fix it. It currently puts the generated metabuild script into the "target" directory (target/.metabuild/metabuild-pkg-hash.rs specifically). However, the "target" dir is unknown when the Target is created. That means that it doesn't know what to put as the src_path in the Target. The current implementation has a hack that will detect the situation later on and use the correct path. I'd like a more elegant solution, but I haven't been able to come up with anything. Currently cargo metadata displays the wrong path (and that may be quite tricky to fix).

~~One alternative is to place the metabuild.rs script in the root of the package. However, I'm uncertain about the possible drawbacks of that approach.~~

The only other idea I had was to try to rewrite the Target list in the Manifest after the "target" directory is known, but I think that will be difficult due to mutability.

Other notes/questions:

Should there be a dedicated namespace in Cargo.toml for metabuild data? If we use package.metadata, then keys may clash with other non-metabuild things.
Is it OK that it doesn't work with rename-dependency? I'm unable to think of a scenario where that would be needed.
I'm not a big fan of toml keys that accept multiple types (string or array in this case), since as a new user they tend to be confusing to me (and it only saves 2 characters). However, I don't feel strongly about it and I implemented it as defined in the RFC.

joshtriplett · 2018-06-11T22:12:47Z

src/cargo/core/compiler/custom_build.rs

@@ -584,7 +626,8 @@ pub fn build_map<'b, 'cfg>(cx: &mut Context<'b, 'cfg>, units: &[Unit<'b>]) -> Ca
        }

        {
-            let key = unit.pkg
+            let key = unit
+                .pkg


This seems like an unrelated formatting change that shouldn't appear in the same commit.

Yea, I prefer to use rustfmt to format, but master has diverged quite a bit from the canonical format. I'll try to remove the unrelated changes.

I was thinking I might post a separate PR running rustfmt, but that touches 123 files which I think probably can't be reviewed properly. I'm not sure how you guys want to handle that moving forward. Each rust release brings more changes to rustfmt. It makes things more difficult the more the master branch diverges.

joshtriplett · 2018-06-11T22:15:06Z

src/cargo/util/toml/targets.rs

 use std::collections::HashSet;
+use std::fs::{self, DirEntry};
+use std::path::{Path, PathBuf};


This seems like an unrelated formatting change. (It's a good change, but unrelated to this commit.)

joshtriplett · 2018-06-11T22:15:19Z

src/cargo/util/toml/targets.rs

 use core::{compiler, Edition, Target};
 use util::errors::CargoResult;
-use super::{LibKind, PathValue, StringOrBool, TomlBenchTarget, TomlBinTarget, TomlExampleTarget,
-            TomlLibTarget, TomlManifest, TomlTarget, TomlTestTarget};


Likewise, unrelated format change.

joshtriplett · 2018-06-11T22:15:52Z

src/cargo/util/toml/targets.rs

+            package_root,
+            edition,
+            legacy_path,
+        );


Unrelated format change.

joshtriplett · 2018-06-11T22:22:32Z

src/cargo/core/compiler/custom_build.rs

+                .find(|u| u.pkg.name().as_str() == name.as_str())
+                .map(|dep| dep.target.crate_name())
+        })
+        .collect();


As a minor nit that I'm not sure needs fixing: this is O(n^2), because it searches all of available_deps for every entry in meta_deps. If some readily available data structure provides an O(1) mapping from name to dep, I'd suggest using that. If one doesn't exist, though, I don't know that it's worth creating one just for this, given that the common case will have relatively few metabuild entries.

I'm unaware of any other way to get the necessary information. As you mention, I would expect these lists to usually be very small.

joshtriplett · 2018-06-11T22:23:40Z

src/cargo/core/compiler/custom_build.rs

+    for dep in &meta_deps {
+        output.push(format!("    {}::metabuild();\n", dep));
+    }
+    output.push("}".to_string());


Minor nit: please include a newline at the end of the file.

joshtriplett · 2018-06-11T22:59:13Z

src/cargo/core/compiler/custom_build.rs

+    };
+    if changed {
+        fs::write(&path, output)?;
+    }


For clarity and potential future reuse, please extract this logic into a "write if changed" function.

joshtriplett · 2018-06-12T00:41:53Z

Thank you for working on this, @ehuss!

joshtriplett · 2018-06-12T21:40:05Z

Other than the issue you mentioned with the target directory, this looks good to me now.

bors · 2018-07-12T16:00:04Z

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

alexcrichton · 2018-07-12T22:38:47Z

Egad I had missed this until now! @ehuss is this ready to go after a once over or were there still any remaining design questions we wanted to answer before landing?

ehuss · 2018-07-12T23:03:24Z

There's one major problem left. The Target::src_path needs to be set to point into the target directory for the metabuild script. However, the target directory is not known until after all the manifests are parsed. Currently there is a hack to adjust the path, but the cargo metadata output is still wrong. I haven't come up with any solutions that don't involve a huge number of changes. The method I was leaning towards is modifying the src_path after parsing is done, but that would involve keeping the manifests mut, and that just sounded really unpleasant. But I haven't tried, yet. I was kinda hoping someone had a simpler idea. 😄

alexcrichton · 2018-07-13T20:45:09Z

Ok sure yeah makes sense. I think though that the cargo metadata output is just gonna need to change somewhat significantly to cope with this. We don't really have a source path to write down in the sense that cargo metadata probably shouldn't be generating files anyway?

ehuss · 2018-07-13T21:35:07Z

Would it be OK if cargo metadata showed the path to the generated metabuild script, even if that file might not exist, yet? (i.e. the file is only created if you run cargo build.)

Example:

{"packages": [
    {"name": "foo",
     "targets": [
        {"kind": ["custom-build"],
         "crate_types": ["bin"],
         "name": "foo",
         "src_path": ["/path/to/foo/target/.metabuild/metabuild-foo-e8a1568fc72dd376.rs"]
        }, 
        ...
     ],
     ...
    }
]}

alexcrichton · 2018-07-15T17:21:35Z

Hm perhaps yeah, I was thinking "no" but I'm also thinking more of build plans. It probably wouldn't be the end of the world for sure! In any case this is all unstable so as long as it's mentioned on the tracking issue it's fine to implement whatever for now I think

bors · 2018-07-16T21:46:29Z

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

ehuss · 2018-07-17T23:37:13Z

I hacked together an alternate solution that mutates the src_path after the workspace is loaded. You can see it here: ehuss@a225f5f In particular, note the fixup() function. I'm not so certain if this is better or not. This new version uses a different path (target/.metabuild/metabuild-NAME-HASH.rs) since the release doesn't affect the file's contents. It also returns a real path in cargo metadata. I also added a test to illustrate that cargo build --build-plan should do the right thing (the generated build script is created), so I think it can still be compatible with external build systems.

The only other approach I've thought of is to change Target::src_path to an Option or some other enum, and then do something different in cargo metadata, but that seems worse to me.

Let me know which approach sounds the most palatable, or if you have some other idea. This new one works a little better (and is a little simpler), so I'm inclined towards that.

alexcrichton · 2018-07-18T19:29:05Z

Thanks! I think though for me at least my preference would be to reflect this in Target itself and just store Option<PathBuf> (effectively) for now. That way it just won't show up in build-plan or metadata output, right? (which is what I think we want the end goal to be?)

ehuss · 2018-07-18T20:51:57Z

(which is what I think we want the end goal to be?)

Hmm, I'm not sure. At least for build-plan, you'd want it to be there, otherwise you wouldn't be able to use this feature with external build systems, right?

As for cargo metadata, what's the reasoning for hiding it? Would it just skip serializing the entire target? (I don't think we can just skip the field or use null, since it would be a breaking change?) I'm not completely familiar with all the use cases. I have a tool that uses src_path, but it would ignore these paths. Are you thinking of tools that inspect these files, and would fail if the file doesn't exist?

alexcrichton · 2018-07-19T19:10:43Z

I'm not sure actually that this would necessarily be compatible with a build plan! In some contexts you may move around where execution happens and the original generation of the plan may happen in a somewhat isolated environment which isn't guaranteed to have changes reflected into others per se. I sort of see this as the responsibility of the build planner to figure out how to assemble this file to compile it, although it's true that the source is always generated by cargo so maybe it is better to include it then?

I think in general I'm just worried about build plans and cargo metadata writing into the target directory when they're in theory read-only commands. That may cause some issues down the road maybe, but I'm not entirely sure.

In any case this is all unstable so I'm fine landing whatever here for now! Just want to make sure we keep tabs on this in the tracking issue

ehuss · 2018-08-01T01:39:43Z

my preference would be to reflect this in Target itself and just store Option (effectively) for now.

I've been trying to figure out how to do this. I'm not sure I understood this suggestion. src_path can't change to an Option since that would be a breaking change for the JSON output, correct? I could filter out the Target from the list in cargo metadata, but Target is serialized in other places (like JSON compiler messages) where there is no alternative, so that might not help.

alexcrichton · 2018-08-01T14:25:11Z

It's a breaking change I think yeah in the sense that consumers who aren't ready for this type of package won't work, but that's also somewhat expected when we add new features to Cargo in that older clients aren't always compatible with newer features. It doesn't break any existing build, though, which I believe is the most important part.

joshtriplett · 2018-08-22T20:52:13Z

cc @wycats @aturon

I'd like to get this unstuck.

ehuss · 2018-08-22T21:35:13Z

Sorry for letting this languish. I would prefer not to break the JSON output, as it would break a lot of tools. I agree it is not perfect to expose the path to a mostly internal file/implementation detail. However, exposing it seems to be less harmful than changing the output. The only concern I can think of is if someone runs cargo metadata before any other command, and expects every source file to exist, it will have a missing file.

Also, for the build-plan portion, I'm not convinced hiding it is a good idea. I don't see how an external build tool would be able to construct the build script and build it properly without the metabuild included in the build-plan output. I think it would essentially make this unusable with external build tools.

alexcrichton · 2018-08-23T18:27:59Z

I basically have no opinion on how this should be implemented, I'm totally fine with basically anything because it's unstable. Just need to be sure to note it down to discuss before stabilization!

- Add newline at end of file. - Remove unnecessary rustfmt changes. - Move file writing code to `write_if_changed`.

- Use new enum `TargertSourcePath` for Target::src_path to make it explicit that metabuild has a special path. - `cargo metadata` now skips the metabuild Target. - JSON artifacts include the true path to the metabuild source file. This may not be the best solution, but it's unclear what it should be, and I would prefer to avoid breaking the output. Alternatively it could just not emit anything? I'm not completely familiar with the use case of these artifact messages. - Place the file in `target/.metabuild/metabuild-pkgname-HASH.rs` instead of in the debug/release directory. Its contents do not depend on the profile. - Fix bug in write_if_changed. - More tests.

ehuss · 2018-08-24T07:02:39Z

I have updated with a new approach of using a custom enum for Target::src_path. I'm not entirely happy with this strategy, because it is a little brittle (particularly around serialization). However, it works better than other approaches I've tried, and avoids changing the JSON structures. In particular:

cargo metadata now skips the metabuild target.
Artifact JSON messages include the true path. I'm not familiar with the use cases for these messages, so I'm not sure if that's really the best approach.
Build plans continue to include the information to build the metabuild script.

I think this is now ready for review, I'm not aware of any remaining issues.

alexcrichton · 2018-08-24T17:14:08Z

@bors: r+

bors · 2018-08-24T17:14:08Z

📌 Commit ecc87b1 has been approved by alexcrichton

alexcrichton · 2018-08-24T17:14:11Z

Thanks!

bors · 2018-08-24T17:14:17Z

⌛ Testing commit ecc87b1 with merge 90fc9f6...

Metabuild (RFC 2196)

bors · 2018-08-24T17:57:15Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 90fc9f6 to master...

rust-highfive assigned alexcrichton Jun 11, 2018

ehuss mentioned this pull request Jun 11, 2018

Tracking issue for RFC 2196, "metabuild: semantic build scripts for Cargo" rust-lang/rust#49803

Open

3 tasks

joshtriplett reviewed Jun 11, 2018

View reviewed changes

src/cargo/util/toml/targets.rs Outdated

package_root,

edition,

legacy_path,

);

Copy link

Member

joshtriplett Jun 11, 2018

Choose a reason for hiding this comment

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

Unrelated format change.

joshtriplett reviewed Jun 11, 2018

View reviewed changes

ehuss force-pushed the metabuild branch from 9b86f6b to 447645b Compare June 12, 2018 21:31

ehuss force-pushed the metabuild branch from 1eafe55 to 0eac1e1 Compare July 12, 2018 18:06

ehuss added 3 commits August 23, 2018 13:31

Metabuild (RFC 2196)

2be857a

Address review comments.

ae5e845

- Add newline at end of file. - Remove unnecessary rustfmt changes. - Move file writing code to `write_if_changed`.

Remove unnecessary change.

e685bbf

ehuss force-pushed the metabuild branch from 0eac1e1 to 62af9aa Compare August 24, 2018 04:55

ehuss force-pushed the metabuild branch from 62af9aa to ecc87b1 Compare August 24, 2018 06:01

ehuss changed the title ~~[WIP] Metabuild (RFC 2196)~~ Metabuild (RFC 2196) Aug 24, 2018

bors added a commit that referenced this pull request Aug 24, 2018

Auto merge of #5628 - ehuss:metabuild, r=alexcrichton

90fc9f6

Metabuild (RFC 2196)

bors merged commit ecc87b1 into rust-lang:master Aug 24, 2018

bors mentioned this pull request Aug 24, 2018

Only use non-absolute paths for path dependencies #5935

Merged

ehuss mentioned this pull request Sep 3, 2018

Update Cargo and RLS rust-lang/rust#53935

Closed

ehuss added this to the 1.30.0 milestone Feb 6, 2022

Metabuild (RFC 2196) #5628

Metabuild (RFC 2196) #5628

Conversation

ehuss commented Jun 11, 2018

rust-highfive commented Jun 11, 2018

ehuss commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtriplett Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtriplett commented Jun 12, 2018

joshtriplett commented Jun 12, 2018

bors commented Jul 12, 2018

alexcrichton commented Jul 12, 2018

ehuss commented Jul 12, 2018

alexcrichton commented Jul 13, 2018

ehuss commented Jul 13, 2018

alexcrichton commented Jul 15, 2018

bors commented Jul 16, 2018

ehuss commented Jul 17, 2018

alexcrichton commented Jul 18, 2018

ehuss commented Jul 18, 2018

alexcrichton commented Jul 19, 2018

ehuss commented Aug 1, 2018

alexcrichton commented Aug 1, 2018

joshtriplett commented Aug 22, 2018

ehuss commented Aug 22, 2018

alexcrichton commented Aug 23, 2018

ehuss commented Aug 24, 2018

alexcrichton commented Aug 24, 2018

bors commented Aug 24, 2018

alexcrichton commented Aug 24, 2018

bors commented Aug 24, 2018

bors commented Aug 24, 2018

ehuss commented Jun 11, 2018 •

edited

Loading

joshtriplett Jun 11, 2018 •

edited

Loading