Do not fail builds with absolute paths in cSources#12005
Conversation
sheaf
left a comment
There was a problem hiding this comment.
The fix looks great. I just made some suggestions to reduce code duplication and improve the documentation.
| @@ -0,0 +1,38 @@ | |||
| cabal-version: 3.14 | |||
| name: setup-hooks-non-hs-nested | |||
There was a problem hiding this comment.
I would much prefer if you updated the existing setup-hooks-non-hs test to use a combination of absolute and relative paths, as opposed to duplicating the entire test.
There was a problem hiding this comment.
Added inner directory with only the cabal.project file to ensure that both in-tree and out-of-tree build scenarios have coverage with just a single test implementation.
My initial attempt at this was adding a rule that always uses an absolute path to generated C source file regardless of how the module is being built.
I have such change on a separate branch here:
https://github.com/e-rk/cabal/blob/9df2bc1839d94038273d620436c7c914cd82622c/cabal-testsuite/PackageTests/SetupHooks/SetupHooksNonHs/SetupHooks.hs#L178-L182
I haven't added it to this PR, because I encountered a different set of problems in that test scenario.
The SetupHooks/Internal.hs build graph implementation might have an unexpected behavior with absolute cSources paths when the autogenComponentModulesDir is relative, but it might have also been a mistake in my test implementation.
I will investigate this further, but I think such test could be added on a separate occasion in a different PR. The question is also whether such scenario is an edge condition that the maintainers consider not worth covering in the first place.
There was a problem hiding this comment.
Hmm, I don't completely follow what the issue is. Could you possibly open a separate ticket about what the issue is? I'd be happy to investigate.
|
Thank you for the review. |
When a build hook supplied a cSources path that was absolute, the linking stage of the build would fail as it expected the paths to be always relative. This poses a problem for Cabal modules that generate C source files at build time. The reason is that when dist-newstyle is located outside the Cabal module tree, such as when it is built as part of a larger cabal.project, the path APIs, such as: - autogenComponentModulesDir - buildDir - ...and so on will produce absolute paths. Normally those APIs produce relative paths when the build directory is located somewhere under the module tree, and therefore don't cause build issues. It is easy for the developer to believe that they should use the above functions to get the path to the build/autogen directory, and only then realize that build completely fails apart at linking stage due to presence of absolute paths under certain circumstances. The change allows the Cabal to process absolute paths in cSources, so the paths produced by above APIs can always be used safely. Added a test that verifies this build scenario is covered. Added changelog entry. Fixes haskell#11998
d33c8e7 to
dfd8bec
Compare
|
Squashed the fixup commits. |
When a build hook supplied a cSources path that was absolute, the linking stage of the build would fail as it expected the paths to be always relative.
This poses a problem for Cabal modules that generate C source files at build time. The reason is that when dist-newstyle is located outside the Cabal module tree, such as when it is built as part of a larger cabal.project, the path APIs, such as:
will produce absolute paths.
Normally those APIs produce relative paths when the build directory is located somewhere under the module tree, and therefore don't cause build issues.
It is easy for the developer to believe that they should use the above functions to get the path to the build/autogen directory, and only then realize that build completely fails apart at linking stage due to presence of absolute paths under certain circumstances.
The change allows the Cabal to process absolute paths in cSources, so the paths produced by above APIs can always be used safely.
Added a test that verifies this build scenario is covered.
Added changelog entry.
Fixes #11998
Template Α: This PR modifies behaviour or interface
significance: significantin the changelog file.