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

For ghc calls pass Cabal's build/ directory after user's src/ directory so that contents of src/ takes precedence #8690

Merged
merged 6 commits into from
Feb 10, 2023

Conversation

sergv
Copy link
Collaborator

@sergv sergv commented Jan 18, 2023

A fix for #8689.

The fix is a one line change that changes order of directories where searches will be searched by the ghc. Cabal's internal build/ directory (e.g. dist-newstyle/build/x86_64-linux/ghc-9.4.4/Cabal-3.9.0.0/build/) now is passed after all source directories specified by the package in order to make sure that if there's a user-supplied file that corresponds to a particular module thet it will never be overshadowed by what's in the cabal's internal build/ directory. Autogenerated modules get written into the internal build/ directory so they have a real chance of shadowing one of user's modules as shown in the linked issue.

Testsing methodology

I built GHC with Cabal that has this patch and run the test. It's tricky to run the added test because it relies on custom setup which can only access Cabal package bundled with GHC.

Custom setup is needed because autogenerated modules with a custom preprocessor (in this case identity preprocessor) are used.


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@sergv sergv force-pushed the source-directory-order branch 2 times, most recently from 18216ef to 46b2d42 Compare January 18, 2023 22:47
@sergv sergv changed the title Source directory order For ghc calls pass Cabal's build/ directory after user's src/ directory so that contents of src/ takes precedence Jan 20, 2023
@sergv sergv requested a review from Mikolaj January 30, 2023 12:04
Copy link
Collaborator

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Looks good, thank you. Please kindly set the merge (squash?) label.

@sergv sergv added the squash+merge me Tell Mergify Bot to squash-merge label Feb 5, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 7, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

The GHA failure is hacked around; let me rebase to fix CI.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

mergify rebase

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

@mergify regresh

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

regresh

❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

refresh

✅ Pull request refreshed

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

silly computer

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

@mergify rebase

@Mikolaj Mikolaj removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 8, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

@sergv: something locked on hard in CI for this PR. Could you kindly rebase manually? Then we'd run CI and I'd force-merge if needed.

@Mikolaj Mikolaj removed the squash+merge me Tell Mergify Bot to squash-merge label Feb 8, 2023
@Mikolaj Mikolaj marked this pull request as draft February 8, 2023 17:18
@Mikolaj Mikolaj marked this pull request as ready for review February 8, 2023 17:18
@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

@mergify requeue

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

requeue

☑️ This pull request is already queued

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

@mergify unqueue

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

unqueue

☑️ The pull request is not queued

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

@mergify requeue

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2023

requeue

☑️ This pull request is already queued

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

doh

@Mikolaj
Copy link
Member

Mikolaj commented Feb 9, 2023

@mergify rebase

sergv added 6 commits February 9, 2023 21:19
Otherwise output will not be correct. It’s tricky to pass proper
Cabal/ version to this test because it needs it for the custom setup.
@mergify
Copy link
Contributor

mergify bot commented Feb 9, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link
Contributor

mergify bot commented Feb 9, 2023

rebase

✅ Branch has been successfully rebased

@cocreature cocreature force-pushed the source-directory-order branch from f6fd7af to fd4f7bf Compare February 9, 2023 21:19
@Mikolaj Mikolaj added 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 labels Feb 9, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Feb 9, 2023

OK, mergify seems to have unclogged

@mergify mergify bot merged commit 664e17d into haskell:master Feb 10, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Feb 10, 2023

Phew, finally. @sergv: thank you again!

@andreasabel
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review 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.

4 participants