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

only check for compiler when project file has conditionals #8358

Merged
merged 7 commits into from
Sep 20, 2022

Conversation

gbaz
Copy link
Collaborator

@gbaz gbaz commented Aug 10, 2022

Should be a simple fix for #8352 -- first check that a project file has conditionals before attempting to fetch the compiler/arch etc info for use in evaluating them. That way, as long as a project file does not have conditionals, actions which did not require ghc to be available (such as sdist) will remain not requiring ghc.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 11, 2022

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2022

rebase

✅ Branch has been successfully rebased

@gbaz
Copy link
Collaborator Author

gbaz commented Aug 11, 2022

Added a test, but it succeeds even when it should fail because I can't set the env to not have ghc in the path properly somehow. Setting it to Nothing makes the system scream about how there's no path at all, but setting it to Just "" doesn't seem to kill the path, just to like not append to it? sigh.

Also I discovered in working on this that sdist's fix to respect project options in #8109 was incomplete and I made a hopefully totally safe change to fix that too...

@ffaf1
Copy link
Collaborator

ffaf1 commented Aug 25, 2022

(live testing as requested in cabal-dev meet)

I can confirm that on my dev machine, with GHC not in PATH, cabal update:

  • in master complains about missing GHC;
  • in gb/compiler-check-logic does not.

@gbaz
Copy link
Collaborator Author

gbaz commented Aug 25, 2022

As per our discussion in the meeting, we don't seem to be able to test with an altered path missing ghc, so I've rolled back the not-working tests and we'll rely on manual verification. After I fix up docs and changelog this is ready for merge.

@ulysses4ever
Copy link
Collaborator

Last minute idea about a test to show that GHC is not needed... I see a bunch of withEnv [("WITH_GHC", Just ghc_path)] in the test suite right now. Perhaps, if you have garbage in ghc_path, it will fall if and only if it's trying to call GHC?

@gbaz
Copy link
Collaborator Author

gbaz commented Aug 25, 2022

good thought, but i just tried that and that didn't work either. Terribly confusing.

@jneira
Copy link
Member

jneira commented Aug 26, 2022

i suppose it relies in the PATH anyways, maybe a withEnv [("PATH", Just ${pathWithoutGHC})] would do the trick

@ulysses4ever
Copy link
Collaborator

@jneira Gershom tried it, see above:

setting it to Just "" doesn't seem to kill the path, just to like not append to it? sigh

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022
@ulysses4ever ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022
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.

If this just physically can't be tested in CI, let's just merge as is. :)

@Mikolaj
Copy link
Member

Mikolaj commented Sep 12, 2022

@gbaz: would you prefer a merge_me or a squash_me?

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 14, 2022

Certainly a squash -- I was hoping to update the docs first, but will not be able to get to it until next week.

@Mikolaj Mikolaj added the squash+merge me Tell Mergify Bot to squash-merge label Sep 16, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Sep 16, 2022

Let me squash if for you, in that case. :)

@Mikolaj
Copy link
Member

Mikolaj commented Sep 19, 2022

Mergify probably balks at the merge delay passed label being tweaked manually (due to mergify script revisions), so we've got one more chance to edit this PR. @gbaz: are we merging or would you like to update the docs first?

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 20, 2022

@Mikolaj added changelog and docs. Please do squash/merge!

@Mikolaj
Copy link
Member

Mikolaj commented Sep 20, 2022

With pleasure.

@Mikolaj Mikolaj added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 20, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Sep 20, 2022

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Sep 20, 2022

rebase

✅ Branch has been successfully rebased

@mergify mergify bot merged commit 1d4491f into master Sep 20, 2022
alexbiehl pushed a commit to alexbiehl/cabal that referenced this pull request Dec 15, 2022
)

* only check for compiler when project file has conditionals

* fix sdist options a bit plus test

* whitespace

* poke tests around

* Update cabal.out

* Update cabal.test.hs

* changelog/docs

Co-authored-by: Gershom Bazerman <[email protected]>
@andreabedini andreabedini deleted the gb/compiler-check-logic branch December 14, 2023 08:15
juhp added a commit that referenced this pull request Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants