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

Can we change componentBuildDir to distinguish different types of components? #9498

Open
sheaf opened this issue Dec 6, 2023 · 5 comments
Open
Assignees

Comments

@sheaf
Copy link
Collaborator

sheaf commented Dec 6, 2023

Currently, we have:

componentBuildDir :: LocalBuildInfo -> ComponentLocalBuildInfo -> FilePath
componentBuildDir lbi clbi =
  buildDir lbi
    </> case componentLocalName clbi of
      CLibName LMainLibName ->
        if prettyShow (componentUnitId clbi) == prettyShow (componentComponentId clbi)
          then ""
          else prettyShow (componentUnitId clbi)
      CLibName (LSubLibName s) ->
        if prettyShow (componentUnitId clbi) == prettyShow (componentComponentId clbi)
          then unUnqualComponentName s
          else prettyShow (componentUnitId clbi)
      CFLibName s -> unUnqualComponentName s
      CExeName s -> unUnqualComponentName s
      CTestName s -> unUnqualComponentName s
      CBenchName s -> unUnqualComponentName s

Ignoring the Backpack-related stuff about libraries, one issue here is that one can end up with the same build directory for a library and an executable of the same name. Instead, I would expect library build dirs to be given by buildDir lbi </> "lib" </> nm and the executable build dirs to be buildDir lbi </> "exe" </> nm.

This potential for clashes was the source of workarounds, e.g. using buildDir lbi </> nm </> nl ++ "-tmp" to avoid the build path for a non-library clashing with the main library build path. I believe this workaround was introduced by commit 56bb1ba.

How feasible would such a change be? I imagine changing the structure of these buildDirs could break assumptions people make about where build products are put, but I think it would be nicer if build products were always simply put in componentBuildDir and that there are no additional workarounds piled on top involving -tmp.

@sheaf
Copy link
Collaborator Author

sheaf commented Dec 6, 2023

I'm wondering if componentBuildDir being different from exeBuildDir and flibBuildDir is a potential source of bugs.

I have found the following potentially problematic uses of componentBuildDir:

  • copyComponent ... (CFLib flib) ... uses componentBuildDir and not flibBuildDir.
  • componentGhcOptions uses componentBuildDir and not exeBuildDir.
  • LibV09.runTest uses componentBuildDir.
    Each of these could be hiding a bug IMO. All of the other uses of componentBuildDir (14 of them) seem OK, as they are on a library.

@mpickering
Copy link
Collaborator

I think the build directory for a component should always be:

buildDir lbi </> prettyShow (componentUnitId clbi)

This is also related to #9389.

@andreabedini
Copy link
Collaborator

Just a note, this does not happen when cabal-install drives a per-component build (dist-newstyle/build/..., dist-newstyle/x/component-name/build/...) but it does happen if cabal-install has to do a whole-package build (because that leaves everything to Cabal).

@andreabedini
Copy link
Collaborator

I agree with @mpickering's suggestion above.

FWIW, I belive that documenting the change and giving a migration path (which in this case could be just be this dir is now that dir) avoids most of the pain.

@sheaf
Copy link
Collaborator Author

sheaf commented Dec 15, 2023

Just a note, this does not happen when cabal-install drives a per-component build (dist-newstyle/build/..., dist-newstyle/x/component-name/build/...) but it does happen if cabal-install has to do a whole-package build (because that leaves everything to Cabal).

So, to make sure I understand:

  • cabal-install passes the configDistPref (and buildDistPref) values, and that feeds into the computation of buildDir and thus of componentBuildDir.
  • when cabal-install is working per-component, it decides on a good base directory, e.g. executable blah goes into dist-newstyle/x/component-name/build rather than just dist-newstyle/build. This avoids the issue with clashes.

Does that mean the following course of action is desired:

  • in Cabal, change componentBuildDir to be buildDir lbi </> prettyShow (componentUnitId clbi), and make sure to use that consistently in the Cabal codebase,
  • in cabal-install, change the computation of distBuildDirectory in some way to avoid including the component unit id twice in the directory?

alt-romes added a commit to mpickering/cabal that referenced this issue Jan 3, 2024
@alt-romes alt-romes self-assigned this Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants