Fix some rust issues #223
Fix some rust issues #223
Conversation
|
So while I was trying to implement support for duplicated versions, I came across to rust-lang/cargo#10310. The package in question has duplicated versions of a dependency in it's lockfile which |
|
Quick question before I review this. Is this PR just working around an upstream issue or does it actually fix bugs on our side? We should probably not increase complexity on our side if this is an upstream issue. |
This PR only fixes bugs on our side. I made an issue on upstream that should handle the issue I mentioned. |
| @@ -251,6 +251,7 @@ in let | |||
| builder, | |||
| builderArgs, | |||
| fetchedSources, | |||
| sourceRoot, | |||
There was a problem hiding this comment.
I think this can be removed. It isn't used by the builders. Also I think that we should change the way we handle relative path sources. I think we should always pass the root source as src and then set sourceRoot of mkDerivation. The unpackPhase will then switch to the correct subdir. That would allow packages to always refer to files in parent directories without issues. Some nodejs builds currently crash because sub-projects want to access files of parent projects in order to build.
There was a problem hiding this comment.
It is used by the Rust builders with this PR, to be able to get the full project repository so path dependencies work properly. Before it would only capture the path dependency directory, which will break if the path dependency depends on other path dependencies that are in a parent directory. We could remove this fine if we removed the builtins.path usage for path depedencies, but I think that would cause regressions?
There was a problem hiding this comment.
True. This is a trade-off situation that should be carefully thought through.
The isolation of sources gives us better incremental builds, but it also breaks some builds. We should probably provide the user with the choice to turn this on or off, and have the default on the safer side which is probably not to do source isolation. At least for nodejs that would be the case. Maybe the default can be different from subsystem to subsystem.
There was a problem hiding this comment.
Yes, I agree. I think we should have different functions for getting an isolated path vs a non-isolated path. And then the builder can decide which one to use.
This works on #219