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

Cabal: Always pass -package-env=- to supported GHC versions #10828

Merged
merged 2 commits into from
Mar 26, 2025
Merged

Conversation

mpickering
Copy link
Collaborator

@mpickering mpickering commented Mar 11, 2025

Issue #10759 highlighted the issue that we were not isolating the calls
to ghc from the existence of environment files.

This manifested in a terminal bug where extra arguments form the
environment file were causing a link failure which was due to a
combination of #10692.

However, even before this bug the test executable was relinked to due to
the extra flags from the environment file.

Building test suite 'aeson-schemas-test' for aeson-schemas-1.4.2.1...
Loaded package environment from /home/runner/work/aeson-schemas/aeson-schemas/dist-newstyle/tmp/environment.-69233/.ghc.environment.x86_64-linux-9.6.6
Loaded package environment from /home/runner/work/aeson-schemas/aeson-schemas/dist-newstyle/tmp/environment.-69233/.ghc.environment.x86_64-linux-9.6.6
[23 of 23] Linking /home/runner/work/aeson-schemas/aeson-schemas/dist-newstyle/build/x86_64-linux/ghc-9.6.6/aeson-schemas-1.4.2.1/t/aeson-schemas-test/build/aeson-schemas-test/aeson-schemas-test [Flags changed]

The correct solution is that calls to ghc made by Cabal should never
implicitly use an environment file. This is similar to how
GHC_PACKAGE_PATH is treated.

Fixes #10759


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

@Mikolaj
Copy link
Member

Mikolaj commented Mar 17, 2025

Can this break users' workflows? IIRC cabal install --lib used to create env files so this change might break the functionality. Were they the same env files? I guess it does not create the files any more due to the confusion it created and bug reports we were getting. Is that right?

@ulysses4ever
Copy link
Collaborator

On one hand, it fixes a regression, on the other: it does seem to have a potential for breaking things. I'm torn on this one..

@mpickering
Copy link
Collaborator Author

@Mikolaj Do you have a test case you are imagining?

The change is about isolating times cabal internally calls ghc, it is already checked if GHC_PACKAGE PATH is set (and cabal fails to operate in those situations, environment files are similar to that.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 18, 2025

@mpickering: my memory of this functionality is too fuzzy, unfortunately, and moreover, it probably got simplified and so your changes are harmless. The idea was that somebody calls cabal install --lib, it creates an environment file and then the user depends on this file, but your change causes GHC to ignore that file. As I said, cabal install --lib probably no longer does this and I probably misremember even that it did, but I hoped maybe, after working on this fix, you know how to easily check if cabal (still?) creates the files, etc.

@mpickering
Copy link
Collaborator Author

A user calling ghc can still use the environment file created by cabal install --lib. A user calling ghc themselves will not be affected by this change, it only affects Cabal and cabal-install invoking ghc during the build process.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 18, 2025

Well, that's good, but it seems at least a year ago users did expect (though some did not and were confused) ghc called by cabal-install to use the env files created by cabal install --lib: #6481 (comment)

@mpickering
Copy link
Collaborator Author

@Mikolaj I skimmed through the thread and there are multiple comments by different people about bugs running cabal inside cabal exec! Things fixed by this patch.

For example, these three issues are fixed by this patch I imagine.

You can see from the test that I added, that if cabal is not isolated from the environment file then it will stop working when run in an environment where GHC_ENVIRONMENT is set. I would advise to merge this patch but not backport it to a minor release.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, let's give users some time to be potentially burned by this change before it's released.

@mpickering mpickering added the merge me Tell Mergify Bot to merge label Mar 24, 2025
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Mar 24, 2025
Issue #10759 highlighted the issue that we were not isolating the calls
to ghc from the existence of environment files.

This manifested in a terminal bug where extra arguments form the
environment file were causing a link failure which was due to a
combination of #10692.

However, even before this bug the test executable was relinked to due to
the extra flags from the environment file.

```
Building test suite 'aeson-schemas-test' for aeson-schemas-1.4.2.1...
Loaded package environment from /home/runner/work/aeson-schemas/aeson-schemas/dist-newstyle/tmp/environment.-69233/.ghc.environment.x86_64-linux-9.6.6
Loaded package environment from /home/runner/work/aeson-schemas/aeson-schemas/dist-newstyle/tmp/environment.-69233/.ghc.environment.x86_64-linux-9.6.6
[23 of 23] Linking /home/runner/work/aeson-schemas/aeson-schemas/dist-newstyle/build/x86_64-linux/ghc-9.6.6/aeson-schemas-1.4.2.1/t/aeson-schemas-test/build/aeson-schemas-test/aeson-schemas-test [Flags changed]
```

The correct solution is that calls to `ghc` made by `Cabal` should never
implicitly use an environment file. This is similar to how
`GHC_PACKAGE_PATH` is treated.

Fixes #10759
The test tries to run `cabal` in an environment where `GHC_ENVIRONMENT`
is set, and checks that the compilation of a simple package isn't
affected by the variable being set.
Copy link
Contributor

mergify bot commented Mar 26, 2025

This pull request has been removed from the queue for the following reason: pull request manually updated.

The pull request #10828 has been manually updated

If you want to requeue this pull request, you can post a @mergifyio requeue comment.

@mergify mergify bot merged commit 9e89bb3 into master Mar 26, 2025
47 checks passed
@mergify mergify bot deleted the wip/t10759 branch March 26, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review don't backport merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period regression in 3.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cabal-install 3.14 linking fails with "shared object file not found"
4 participants