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

Resolve symlinks for cargo and xargo home. #947

Merged
merged 1 commit into from
Jul 16, 2022

Conversation

Alexhuszagh
Copy link
Contributor

@Alexhuszagh Alexhuszagh commented Jul 14, 2022

Resolve symlinks for the xargo and cargo home (as well as the Nix store) prior to mounting, since they are mounted at a fixed location anyway. This is because podman mounts symlinks as root by default. We always canonicalize the paths on the host, after creating them, to ensure that any symlinks are resolved.

Say we have /path/to/home, and /path/to is a symlink but /path/to/home doesn't exist yet. Trying to canonicalize /path/to/home/.xargo will fail, and will still be a symlink if we maybe canonicalize before. First creating the directories and canonicalizing them on the host should always work.

Change the mount points from /cargo, /xargo, and /rust to the same paths as on the host, which avoids unnecessary recompilation when cargo and cross are intermittently used.

Closes #280.
Closes #373.
Closes #551.

@Alexhuszagh Alexhuszagh requested a review from a team as a code owner July 14, 2022 22:54
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Jul 14, 2022
947: Resolve symlinks for cargo and xargo home. r=Emilgardis a=Alexhuszagh

Resolve symlinks for the xargo and cargo home (as well as the Nix store) prior to mounting, since they are mounted at a fixed location anyway. This is because podman mounts symlinks as root by default.

Closes #373.
Doesn't interfere with #920 and #942, although #942 might need work to ensure non-canonical paths are mounted with normal permissions.

Co-authored-by: Alex Huszagh <ahuszagh@gmail.com>
@Emilgardis
Copy link
Member

bors r-

@bors
Copy link
Contributor

bors bot commented Jul 14, 2022

Canceled.

@Emilgardis
Copy link
Member

canonicalization is only needed on xargo when envvar is set (and exists)

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 14, 2022

Yeah, I planned on converting this to a draft before my internet died. This still fails right now if the path exists but it's a broken symlink.

@Alexhuszagh Alexhuszagh marked this pull request as draft July 14, 2022 23:33
@Alexhuszagh Alexhuszagh marked this pull request as ready for review July 15, 2022 04:22
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 15, 2022

I think this is ready: it fails if the symlink exists but is broken, but that's a feature I believe, and will error with the following warning:

Error: 
   0: couldn't create directory "/home/ahuszagh/Desktop/cross/path/to/registry/.xargo"
   1: File exists (os error 17)

.changes/947.json Show resolved Hide resolved
.changes/947.json Outdated Show resolved Hide resolved
src/docker/remote.rs Outdated Show resolved Hide resolved
src/docker/remote.rs Outdated Show resolved Hide resolved
@Emilgardis
Copy link
Member

This is a cool change, it also makes it more clear in verbose build that you're actually running a binary mounted from your host system, since /my/home/.rustup/toolchain/toolchain is more prevalent now

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

lgtm! overall a huge improvement in ux

I feel like we should make some path shims though, to easily descern between what's on the host and what is in the image, that can be done separately though.

Some small things left

src/docker/remote.rs Outdated Show resolved Hide resolved
src/docker/remote.rs Outdated Show resolved Hide resolved
src/docker/remote.rs Outdated Show resolved Hide resolved
src/docker/remote.rs Outdated Show resolved Hide resolved
src/docker/shared.rs Outdated Show resolved Hide resolved
@Emilgardis
Copy link
Member

test_host is failing due to stable-x86_64-unknown-linux-gnu not existing, I think it's fine to pre-install this toolchain in the test job (only in the test job though)

@Alexhuszagh
Copy link
Contributor Author

test_host is failing due to stable-x86_64-unknown-linux-gnu not existing, I think it's fine to pre-install this toolchain in the test job (only in the test job though)

We might need a better strategy. Let me think...

@Alexhuszagh Alexhuszagh force-pushed the podman_symlink branch 2 times, most recently from f30fdf7 to 63829b5 Compare July 16, 2022 16:00
Resolve symlinks for the xargo and cargo home (as well as the Nix store) prior to mounting, since they are mounted at a fixed location anyway. This is because podman mounts symlinks as root by default. We always canonicalize the paths on the host, after creating them, to ensure that any symlinks are resolved.

Say we have `/path/to/home`, and `/path/to` is a symlink but `/path/to/home` doesn't exist yet. Trying to canonicalize `/path/to/home/.xargo` will fail, and will still be a symlink if we maybe canonicalize before. First creating the directories and canonicalizing them on the host should always work.

Change the mount points from `/cargo`, `/xargo`, and `/rust` to the same paths as on the host, which avoids unnecessary recompilation when `cargo` and `cross` are intermittently used.
@Alexhuszagh
Copy link
Contributor Author

Ok I've changed this to use the host target triple (only for the unittest), so the toolchain will always be installed.

@Alexhuszagh
Copy link
Contributor Author

bors r=Emilgardis

@bors
Copy link
Contributor

bors bot commented Jul 16, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants