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 during unpacking #233

Closed
wants to merge 1 commit into from
Closed

Resolve symlinks during unpacking #233

wants to merge 1 commit into from

Conversation

Patryk27
Copy link
Contributor

@Patryk27 Patryk27 commented Apr 16, 2022

Closes #230

Abstract

It's one of those one-byte-changes - during Git dependency unpacking, instead of:

cp -r $(dirname $toml) $dest

... what I'm proposing to do instead is:

cp -rL $(dirname $toml) $dest

... where -L tells cp to resolve symlinks - i.e. after this operations, symlinks will be substituted with the contents of the files they point at.

Overall, this will allow us to correctly compile packages such as maud that contain cross-workspace-package symlinks (e.g. this boi that points outside of maud_macros).

Without this fix, our new test would fail with:

error: couldn't read /nix/store/...-crates-io/dep/src/../symlink.txt: No such file or directory (os error 2)
 --> /sources/.../src/lib.rs:2:5
  |
  |     include_str!("../symlink.txt")
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

... and a similar error would be generated for maud, as can be seen in issue linked at the top.

Potential downsides

This fix resolves all symlinks - in reality, it should be enough to resolve only those symlinks that point outside of the currently processed crate; in my opinion that'd make the implementation more difficult for no apparent gain, though.

Caveats

For clarity, this pull request depends on #232; after that one gets merged, this PR should remain with just one commit inside of it.

@blitz
Copy link
Member

blitz commented Apr 18, 2022

@Patryk27 I've merged #232. Can you rebase this PR?

@Patryk27
Copy link
Contributor Author

Ready! 🚀

cd $out
cp -ar ${./test/git-symlink/app}/* .

depPath="${dep}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is the same as for git-dep-by-branch-with-slash - for simplicity, I'm planning on refactoring it (i.e. extracting to some common function) in a separate commit later, after this PR.

@Patryk27
Copy link
Contributor Author

In the meantime I've found a much better way to approach this issue, so let me close this merge request and open a new one 🙂

@Patryk27 Patryk27 closed this Apr 19, 2022
@Patryk27 Patryk27 deleted the bug/symlinks branch June 24, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symlinks are not resolved after a submodule is cloned
2 participants