Remove symbolic link to a file out side the crate

[yanganto]

Hi there,

When building with lighting in nix, There is an issue with lightning-invoice

The symbolic link in lightning-invoice points to a file under lightning, which is not under lightning-invoice.
https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-invoice/src/time_utils.rs

Could we change to time_utils as a crate, and both lightning and lightning-invoice explicitly import it in Cargo.toml.

time_utils = {path = "../time_utils"}

If this looks good from your end, I can open a PR on this. Thanks. 🙏

[TheBlueMatt]

I'm not sure I understand why this is an issue. Can you elaborate more why nix can't handle symlinks?

[yanganto]

Yes, I miss some context, let me put the details in.

We use crane for packing projects and use the commit hash for dependency, not from crate.io like this.
https://github.com/kuutamolabs/lightning-knd/blob/e398cdc31aacb9089b88a862dc0a11fba76eb1da/kld/Cargo.toml#L21-L26

This is the failure result of CI job
https://github.com/kuutamolabs/lightning-knd/runs/14673764835

And following is the critical log with replacing hash to xxx and putting some newline, so it will be easier for you to read.

       >   --> /nix/store/xxx-vendor-cargo-deps/xxx/lightning-invoice-0.23.0/src/lib.rs:33:1
       >    |
       > 33 | pub(crate) mod time_utils;
       >    | ^^^^^^^^^^^^^^^^^^^^^^^^^^
       >    |
       >    = help: to create the module `time_utils`,
       create file "/nix/store/xxx-vendor-cargo-deps/xxx/lightning-invoice-0.23.0/src/time_utils.rs" 
         or 
       "/nix/store/xxx-vendor-cargo-deps/xxx/lightning-invoice-0.23.0/src/time_utils/mod.rs"
      

And we can work through it with additional tricks on this.
https://github.com/kuutamolabs/lightning-knd/blob/629c2c984edeba575c6b77ffb4f36fe5e574c1ee/nix/pkgs/kld.nix#L35-L44

The issue will not be in a stable release　when pulling dependency from crate.io, but it is nice for people who want to go close to your main branch. 🙏

[TheBlueMatt]

Hmm, that sounds like it should be reported as a bug to crane - its perfectly reasonable to want to have the same util methods in multiple workspace crates, and symlinks are supported by git and cargo. crane barfing on them is a crane bug.

[yanganto]

This symlink is nothing terrible now, and we can reuse code quickly.
If reusing code explicitly will be great if we can.

There already are CVEs in Rust because of symlinks.

• https://www.cve.org/CVERecord?id=CVE-2022-21658
• https://nvd.nist.gov/vuln/detail/CVE-2022-36113

The symlink is pointing outside of the crate, I am not sure there will be a way to inject codes from this symlink.

If you insist this is nothing worth changing, you can close this issue. This is good from my end, we have a way to work around this in our project.

[TheBlueMatt]

There already are CVEs in Rust because of symlinks.

Those are pretty different contexts - in stdlib not rustc :)

The symlink is pointing outside of the crate, I am not sure there will be a way to inject codes from this symlink.

Right, but the symlink is pointing inside the workspace, which is really what should be operated on when doing a from-git build - the full repo should be cloned and then built from there, rather than just trying to fetch the full repo and copy only the one crate over into the build area. I don't think you should be having to work around this, this is a bug in crane.

If you insist this is nothing worth changing, you can close this issue. This is good from my end, we have a way to work around this in our project.

I don't think I'm saying quite that, but just "symlinks in a repo are bad" isn't all that compelling to me - all the normal rust toolchain things support it, and workspaces with multiple crates are a core part of cargo, so I'm kinda dubious that its bad practice.

Rather, if there's some other reason its bad practice aside from simply "symlinks across crates are bad", I'm certainly open to changing things, but that my itself doesn't seem like an issue to me.

[TheBlueMatt]

Did you get a chance to report this upstream to crane @yanganto?

[yanganto]

No, I do not report this to crane, because there already is a way to work around this. Thank you to take care of this.

The way crane treat Rust crate is good for me. It makes sure every file needs to be under the crate folder. If any special treatment on the crate (for example, using other files out of the crate here), a patch on the build command explicitly will be better than using the file out of the crate implicitly.

It is good to close or put won't fix flag on it, if you think this is not a bug and it can save time once others have a similar issue.

[TheBlueMatt]

Still seems to me that this is a crane bug - if crane is going to operate by looking at git repos, it needs to operate at the repo/rust workspace level, operating at the crate level while fetching from a git repo makes very little sense.