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

Expose srcSubDir and srcSubDirPath in the derivation #1077

Closed
purefn opened this issue Mar 19, 2021 · 18 comments
Closed

Expose srcSubDir and srcSubDirPath in the derivation #1077

purefn opened this issue Mar 19, 2021 · 18 comments

Comments

@purefn
Copy link
Contributor

purefn commented Mar 19, 2021

Our project (over?)uses file-path-th to get paths to files, mostly in tests. It uses some template haskell to get absolute paths to files in the project at compile time. When used with stack and cabal, this is fine. With nix, this poses a problem, because the path that existed at compile time won't exist at runtime. In the past, I applied a patch to file-path-th to lookup the src environment variable and use that instead of the current working directory of the build. That worked great.

But we are trying to update the version of haskell.nix we're using and some of the restructuring has broken that method. Now, instead of component source being at /nix/store/xxxxxxx-mypackage-lib-mycomponent, it is instead at /nix/store/xxxxxxx-mypackage-lib-mycomponent/mycomponent.

I've come up with two approaches to solves this problem:

  1. update the file-path-th patch to find the subdirectory by finding the subdir that contains either a package.yaml file or a .cabal file. This appears to work just fine, but it does incur a slight performance penalty at compile time that I'd like to avoid. This brings me to the reason for this request...
  2. If we add srcSubDir and srcSubDirPath (or, even just one of them), as I've done here https://github.com/Simspace/haskell.nix/commit/5c6363a32bbc23970bc1c583450333285ee3d2e7, then the file-path-th can be changed to use those and we avoid the need to search for the component directory.

This is kind of a special case, which is why I'm opening an issue before a PR, to see if it would be generally acceptable or if there is another way I might be missing.

@hamishmack
Copy link
Collaborator

I assume it is /nix/store/xxxxxxx-mypackage-lib-mycomponent/mypackage (not /mycomponent).

Could you make the files you need available to the component by adding a relative path to extra-source-files in mypackage/mypackage.cabal:

extra-source-files: ../otherpackage/some-file

This should make them available to all components in mypackage though so you might want to use:

modules = [{
  packages.mypackage.components.libs.mycomponent.extraSourceFiles = [ "../otherpackage/some-file" ];
}];

That should result in /nix/store/xxxxxxx-mypackage-lib-mycomponent containing both mypackage and otherpackage/some-file.

@hamishmack
Copy link
Collaborator

Both srcSubDir and srcSubDirPath should already be available on the derivation as they are in passthru

srcSubDir = cleanSrc.subDir;
srcSubDirPath = cleanSrc.root + cleanSrc.subDir;

@hamishmack
Copy link
Collaborator

Oh! I see you want them as env vars when the builder runs.

@hamishmack
Copy link
Collaborator

With nix, this poses a problem, because the path that existed at compile time won't exist at runtime.

Have you tried:

modules = [{
  packages.mypackage.components.libs.mycomponent.keepSource = true;
}];

@purefn
Copy link
Contributor Author

purefn commented Mar 19, 2021

Oh! I see you want them as env vars when the builder runs.

Right, so we can use lookupEnv srcSubDirPath in the TH. Before we could just use lookupEnv src because that pointed at the root of the component source tree. Now it points to the root of the filtered project source tree and there is no way, from within the TH, to figure out what the component root is other than to search for a sub dir with a package.yaml or .cabal file. In practice, this works and introduces only a minor performance hit because we usually have only one, two or three sub directories to look at, e.g. if the component being built is in subsystem/libs/db. But avoiding that would be better.

@purefn
Copy link
Contributor Author

purefn commented Mar 19, 2021

With nix, this poses a problem, because the path that existed at compile time won't exist at runtime.

Have you tried:

modules = [{
  packages.mypackage.components.libs.mycomponent.keepSource = true;
}];

The paths in the libs/exes that were created by TH would still be pointing at /build/, so I don't think that would help here.

@purefn
Copy link
Contributor Author

purefn commented Mar 19, 2021

Hmm this is also a problem with file-embed and the makeRelativeToProject function. If used in a function in one lib, then use it from a lib in another package, the links it creates are to /build too, so it need to have a similar patch applied.

@hamishmack
Copy link
Collaborator

keepSource should use the .source output instead of /build is copies the source files in then switches to that directory in configurePhase.

(lib.optionalString keepSource ''
cp -r . $source
cd $source
chmod -R +w .
'') + ''

@hamishmack
Copy link
Collaborator

There keepSource does have an issue with dist/build/autogen source files as they are currently removed (with an rm -rf dist) after the build completes.

That is something we had to address in the cabal-doctest PR here.

I am not happy with that fix though because it has to remove all the paths that point to $out first (IIRC the cabal generated Paths module is at fault). I think we should consider replacing the $source output with $out/source instead to resolve the circular reference issue.

I also wonder if keepSource should the default.

@purefn
Copy link
Contributor Author

purefn commented Mar 19, 2021

Oooh 👍 . I completely missed that change of directory before building. I'll give it a try! Thanks!

@purefn
Copy link
Contributor Author

purefn commented Mar 19, 2021

It looks like we can't make keepSource the default without a bit of work. Checks fail because the first thing in the check build phase is changing into the sub directory, https://github.com/Simspace/haskell.nix/blob/9d63e57aba2ede9e80cfd7d25a4093785453c400/lib/check.nix#L34. But when using keepSource, there is no sub directory. I would have expected the if statement to fail, but it does not.

@purefn
Copy link
Contributor Author

purefn commented Mar 20, 2021

Looks like maybe https://github.com/Simspace/haskell.nix/blob/9d63e57aba2ede9e80cfd7d25a4093785453c400/lib/check.nix#L33 should be

      lib.optionalString (!(hasAttr "source" drv) && hasAttr "srcSubDir" drv)) ''

@purefn
Copy link
Contributor Author

purefn commented Mar 20, 2021

Oof patched that up and the next thing I'm running into is nix reporting a cycle.

cycle detected in the references of '/nix/store/vszkdr9zyaz25wzn2rng75i3jlb3rjf7-mto-testlib-lib-mto-testlib-1.34.0' from '/nix/store/yr502iy7bzh79lqf9scrba45qpc160pp-mto-testlib-lib-mto-testlib-1.34.0-source'

Looks like this is because the library derivation output has a reference to the source output, and the source output contains a .conf file that points to the library derivation output.

Update: I changed https://github.com/Simspace/haskell.nix/blob/9d63e57aba2ede9e80cfd7d25a4093785453c400/builder/comp-builder.nix#L459 to rm -fr dist ${name}.conf. We'll see how that goes.

@hamishmack
Copy link
Collaborator

It looks like we can't make keepSource the default without a bit of work. Checks fail because the first thing in the check build phase is changing into the sub directory

This was fixed in #427 (which I have just merged) by moving the keepSource copy from configurePhase to prePatch before the cd srcSubDir. That way the .source output does include the directory structure of the project (so it can also include any extra-source-files from other packages).

@hamishmack
Copy link
Collaborator

Update: I changed https://github.com/Simspace/haskell.nix/blob/9d63e57aba2ede9e80cfd7d25a4093785453c400/builder/comp-builder.nix#L459 to rm -fr dist ${name}.conf. We'll see how that goes.

Now I have merged #427 it might be better to change this bit...

# Keep just the autogen files and package.conf.inplace package
# DB (probably empty unless this is a library component).
# We also have to remove any refernces to $out to avoid
# circular references.
if configureAllComponents
then ''
mv dist dist-tmp-dir
mkdir -p dist/build
mv dist-tmp-dir/build/${componentId.cname}/autogen dist/build/
mv dist-tmp-dir/package.conf.inplace dist/
remove-references-to -t $out dist/build/autogen/*
rm -rf dist-tmp-dir
''
else lib.optionalString keepSource ''
rm -rf dist
''

So that to keepSource does that same as configureAllComponents

@purefn
Copy link
Contributor Author

purefn commented Mar 22, 2021

Now I have merged #427 it might be better to change this bit... So that to keepSource does that same as configureAllComponents

Not entirely sure I follow. Do you mean that we should change that to something more like optionalString (configureAllComponent || keepSource) instead of having the separate if/else bits? If so, I'm happy to make a PR for that.

@hamishmack
Copy link
Collaborator

Not entirely sure I follow. Do you mean that we should change that to something more like optionalString (configureAllComponent || keepSource) instead of having the separate if/else bits? If so, I'm happy to make a PR for that.

Yes, that was what I had in mind. I think it is reasonable that keepSource should keep configure time generated source.

@purefn
Copy link
Contributor Author

purefn commented Mar 23, 2021

Not entirely sure I follow. Do you mean that we should change that to something more like optionalString (configureAllComponent || keepSource) instead of having the separate if/else bits? If so, I'm happy to make a PR for that.

Yes, that was what I had in mind. I think it is reasonable that keepSource should keep configure time generated source.

@hamishmack Opened up #1080. I've tested it out and with that change keepSource handles our needs perfectly, so this issue can be closed when that is merged.

Thanks!

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

No branches or pull requests

2 participants