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

Search for root manifest with ephemeral workspaces #7768

Merged
merged 9 commits into from Jan 27, 2020

Conversation

@chrisduerr
Copy link
Contributor

chrisduerr commented Jan 6, 2020

Fixes #5495.

This seems like it's too simple to just work like this, but after trying a few different things, this was the only solution which worked reliably for me.

I've verified that no /target is present in the actual checkout location, the target directory used is actually the one created in /tmp.

I've also verified that both workspaces and "normal" packages still install through git and that a normal cargo install --path works too (though that doesn't use ephemeral workspaces anyways).

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Jan 6, 2020

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 6, 2020

Thanks, looks like there's some test errors?

Also, these changes make the ephemeral function do the opposite of what its doc comment says it does. We also probably don't want it to behave this way for registry packages. Perhaps the logic needs to go in the install_one? There should also be a test for the new behavior.

@chrisduerr

This comment has been minimized.

Copy link
Contributor Author

chrisduerr commented Jan 6, 2020

@ehuss I wasn't quite sure if this is the right approach to address this and if searching for the root from a child manifest would create issues in an ephemeral workspace.

How would this be implemented in the install_one method? I've tried playing around with that first, but the install_one method has only one concept of manifests. So if the virtual manifest is used in the install_one method to create the ephemeral manifest, it's not possible to run cargo install, since it will result in a 'manifest is virtual manifest' error.

So based on my very surface-level understanding, it seems like the root manifest and manifest itself need to be configured separately for it to track things correctly.

Maybe it's possible to add a flag to the ephemeral workspace creation which tells it if the root manifest should be searched for?

@chrisduerr

This comment has been minimized.

Copy link
Contributor Author

chrisduerr commented Jan 6, 2020

I've just looked at the failing testcase and it was a test that used install for two git dependencies in a workspace. However the workspace manifest was actually malformed, so the test itself was faulty.

I'll see if I can add another test that explicitly makes sure that the workspace manifest is respected in a git install, but it seems like no existing test fails with this change anymore (except for one that apparently should fail?).

@chrisduerr

This comment has been minimized.

Copy link
Contributor Author

chrisduerr commented Jan 6, 2020

I've also added a test that makes sure an error in the virtual manifest leads to an error when trying to install from git. Unfortunately I didn't see a cleaner way to verify that the virtual manifest is actually read. But the test does indeed fail on the latest master, while passing on this PR.

Also, these changes make the ephemeral function do the opposite of what its doc comment says it does.

I'd like some more feedback now that tests are fixed and a new test is written, before changing the documentation of the ephemeral constructor, since doc changes wouldn't be needed when changes are made in a different location. However as far as I can tell the workspace would still have only one package, but I might be misunderstanding this comment.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 13, 2020

Taking a closer look, it seems like you want cargo install --git to honor a workspace? I was a bit confused because #5495 was talking about --path. --path already honors workspaces. I think to make it also work for git repos, this line would need to be changed to also check is_git().

I haven't thought too much about the consequences of that.

@chrisduerr

This comment has been minimized.

Copy link
Contributor Author

chrisduerr commented Jan 13, 2020

@ehuss According to Alex:

I think it makes sense for git and path deps yeah, but not for crates.io

That's why I went ahead trying to look into this, since it is required to make git installs work at all when you're running with a workspace.

Alacritty currently uses workspaces and it is impossible to install it through git directly.

I think to make it also work for git repos, this line would need to be changed to also check is_git().

I've tried that, but the result of that patch would be that the target directory will be in the repository of the checked out project. So it would be in ~/.cargo/git/checkouts/..., which seems very undesirable for the hard drive space of cargo users.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 13, 2020

I've tried that, but the result of that patch would be that the target directory will be in the repository of the checked out project.

I think it would be fine to rewrite those lines to decouple the target_dir and whether or not to use an ephemeral workspace. I think for a git source, it could use the same temp dir logic. You might need to add a set_target_dir method to Workspace.

Another thing to be careful with is whether or not the Cargo.lock file is written. I think if require_optional_deps is set, it will not be written, so that should be fine.

@chrisduerr

This comment has been minimized.

Copy link
Contributor Author

chrisduerr commented Jan 14, 2020

I've tried using the non-ephemeral workspace, however it seems a lot more convoluted than before and it does not yet work properly.

Since we need to check for updates and a normal workspace usually does not have a package, it is regarded as path by default, which means that the binary is always replaced. I've tried to make it understand that it's a git depedency by adding the set_package function, however that introduced the problem that multi-crate git dependencies (git_with_lockfile test) cannot be resolved anymore.

I've also just started trying to copy-paste things over from the ephemeral workspace to get this working, without much success, but that doesn't seem like the correct approach here, since we specifically want to get away from ephemeral.

Is there a better way to let check_update know that the workspace package is a git depedency, so it performs its proper checks?

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 18, 2020

Hm, I think I see what you mean. It looks like to me the problem is this line which resets pkg. If you can restructure the code so that is not called if the source is a git or path (and remove the call to set_package), I think that should work.

@chrisduerr

This comment has been minimized.

Copy link
Contributor Author

chrisduerr commented Jan 18, 2020

Hm, I think I see what you mean. It looks like to me the problem is this line which resets pkg. If you can restructure the code so that is not called if the source is a git or path (and remove the call to set_package), I think that should work.

Thanks, I'll give it a shot and report back if I run into any more troubles.

chrisduerr added 5 commits Jan 6, 2020
Fixes #5495.
@chrisduerr chrisduerr force-pushed the chrisduerr:install-workspaces-from-git branch from 492e192 to 483ad7c Jan 19, 2020
@chrisduerr

This comment has been minimized.

Copy link
Contributor Author

chrisduerr commented Jan 20, 2020

If you can restructure the code so that is not called if the source is a git or path (and remove the call to set_package), I think that should work.

Not calling it when it's a git path does indeed work. For a "normal" path it should not be necessary since we're not doing the set_target_dir with the new tempdir either.

I think the behavior should now be implemented in the way you've suggested, please let me know if you were thinking about a different approach.

Since we're talking about borrowing here with the &Package it is a bit complicated to make sure that everything is nice and clean, since we can't just return the referenced package from the match statement. I've chosen to use an Option to work around this, which certainly isn't ideal, but it should be a relatively clean minimal change.

Another thing to be careful with is whether or not the Cargo.lock file is written. I think if require_optional_deps is set, it will not be written, so that should be fine.

It indeed doesn't appear to write a lockfile to the git checkouts cache, so I think that should be fine.

I've noticed that the /tmp/cargo-... directory wasn't properly deleted however, so I've copied the behavior from the other TempFileBuilder, which also sets td_opt to prevent cleanup when the installation fails. I feel like it makes sense to have the same behavior here, so please let me know if you disagree.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 23, 2020

Yea, getting the reference out is a bit awkward.

To avoid some of the temp code duplication, I would flip the logic around so that the workspace is created first, and then decide which target dir to use. Maybe something like this:

    let mut td_opt = None;
    let mut needs_cleanup = false;
    let mut git_package = None;
    let mut ws = if source_id.is_git() || source_id.is_path() {
        let mut ws = Workspace::new(pkg.manifest_path(), config)?;
        ws.set_require_optional_deps(false);
        if source_id.is_git() {
            // Don't use ws.current() in order to keep the package source as a
            // git source so that install tracking uses the correct source.
            git_package = Some(&pkg);
        }
        ws
    } else {
        Workspace::ephemeral(pkg, config, None, false)?
    };
    if !source_id.is_path() {
        let target_dir = if let Some(dir) = config.target_dir()? {
            dir
        } else if let Ok(td) = TempFileBuilder::new().prefix("cargo-install").tempdir() {
            let p = td.path().to_owned();
            td_opt = Some(td);
            Filesystem::new(p)
        } else {
            needs_cleanup = true;
            Filesystem::new(config.cwd().join("target-install"))
        };
        ws.set_target_dir(target_dir);
    }
    ws.set_ignore_lock(config.lock_update_allowed());
    let pkg = git_package.map_or_else(|| ws.current(), |pkg| Ok(pkg))?;
let (mut ws, git_package) = if source_id.is_git() {
// Don't use ws.current() in order to keep the package source as a git source so that
// install tracking uses the correct sourc.
(Workspace::new(pkg.manifest_path(), config)?, Some(&pkg))
} else if source_id.is_path() {
(Workspace::new(pkg.manifest_path(), config)?, None)
} else {
needs_cleanup = true;
Some(Filesystem::new(config.cwd().join("target-install")))
};

let mut ws = match overidden_target_dir {
Some(dir) => Workspace::ephemeral(pkg, config, Some(dir), false)?,
None => {
let mut ws = Workspace::new(pkg.manifest_path(), config)?;
ws.set_require_optional_deps(false);
ws
}
(Workspace::ephemeral(pkg, config, None, false)?, None)
};
ws.set_ignore_lock(config.lock_update_allowed());
let pkg = ws.current()?;
ws.set_require_optional_deps(false);
Comment on lines 203 to 213

This comment has been minimized.

Copy link
@chrisduerr

chrisduerr Jan 24, 2020

Author Contributor

I'd love to know how you feel about this specific change @ehuss. I feel like generally it's a bit more Rust-y, but it does come with the disadvantage that the set_require_optional_deps(false) is basically a pointless no-op for the ephemeral workspace since that is already created with that argument.

But generally I feel like this slims down the different cases a bit and makes it a little more obvious maybe that we're looking at three mostly different variations, rather than two variation where one has another sub-variation? Both seem fine, but I think I'd personally prefer this approach.

This of course also has the benefit that the git_package is not mutable anymore and only ever created with the value that it's also going to be used with.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 27, 2020

Looks great! I pushed a minor typo fix.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 27, 2020

📌 Commit 601d04c has been approved by ehuss

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 27, 2020

⌛️ Testing commit 601d04c with merge 328b7d6...

bors added a commit that referenced this pull request Jan 27, 2020
Search for root manifest with ephemeral workspaces

Fixes #5495.

This seems like it's too simple to just work like this, but after trying a few different things, this was the only solution which worked reliably for me.

I've verified that no `/target` is present in the actual checkout location, the target directory used is actually the one created in `/tmp`.

I've also verified that both workspaces and "normal" packages still install through git and that a normal `cargo install --path` works too (though that doesn't use ephemeral workspaces anyways).
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 27, 2020

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 328b7d6 to master...

@bors bors merged commit 601d04c into rust-lang:master Jan 27, 2020
11 checks passed
11 checks passed
homu Test successful
Details
rust-lang.cargo Build #20200127.2 succeeded
Details
rust-lang.cargo (Linux beta) Linux beta succeeded
Details
rust-lang.cargo (Linux nightly) Linux nightly succeeded
Details
rust-lang.cargo (Linux stable) Linux stable succeeded
Details
rust-lang.cargo (Windows x86_64-msvc) Windows x86_64-msvc succeeded
Details
rust-lang.cargo (build_std) build_std succeeded
Details
rust-lang.cargo (docs) docs succeeded
Details
rust-lang.cargo (macOS) macOS succeeded
Details
rust-lang.cargo (resolver) resolver succeeded
Details
rust-lang.cargo (rustfmt) rustfmt succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.