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

Fix concurrency bug in test suite #6850

Merged
merged 13 commits into from
Sep 15, 2023
Merged

Fix concurrency bug in test suite #6850

merged 13 commits into from
Sep 15, 2023

Conversation

lawcho
Copy link
Contributor

@lawcho lawcho commented Sep 14, 2023

Fixes #6832

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks!
Not manipulating a global env is indeed much better than what we have now!

I have a few comments.

src/full/Agda/Utils/Environment.hs Outdated Show resolved Hide resolved
test/Utils.hs Outdated Show resolved Hide resolved
src/full/Agda/Utils/Environment.hs Outdated Show resolved Hide resolved
@andreasabel andreasabel modified the milestones: 2.6.4, 2.6.5 Sep 14, 2023
src/full/Agda/Utils/Environment.hs Outdated Show resolved Hide resolved
@andreasabel andreasabel added the infra: test suite Issues relating to the test suite (not in changelog) label Sep 14, 2023
@andreasabel andreasabel added the pr: squash-me This PR needs squashing label Sep 14, 2023
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

I suggest some cosmetic fixes.

  • Pushed a commit with cosmetics in the HACKING file---but actually wanted to use the suggestion feature.
  • Used the suggestion feature. You can batch the suggestions and then apply them together.

src/full/Agda/Utils/Environment.hs Outdated Show resolved Hide resolved
src/full/Agda/Utils/Environment.hs Outdated Show resolved Hide resolved
src/full/Agda/Utils/Environment.hs Outdated Show resolved Hide resolved
test/Utils.hs Outdated Show resolved Hide resolved
@andreasabel andreasabel modified the milestones: 2.6.5, 2.6.4 Sep 14, 2023
Co-authored-by: Andreas Abel <andreas.abel@ifi.lmu.de>
@andreasabel andreasabel self-assigned this Sep 15, 2023
@andreasabel andreasabel merged commit 7a40be8 into agda:master Sep 15, 2023
23 checks passed
-- hPutStrLn stderr $ unwords $ agdaBin : envArgs ++ args
let agdaProc = (proc agdaBin (envArgs ++ args)) { create_group = True }
let agdaProc = (proc (getAgdaBin env) (envArgs ++ args)) { create_group = True , env = Just env }
Copy link
Member

@andreasabel andreasabel Oct 7, 2023

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

So this is likely a bug in the process library that only surfaces on macOS. The fix of this bug suggests that setting cwd here would workaround this bug, see https://github.com/haskell/process/pull/296/files.

Copy link
Member

Choose a reason for hiding this comment

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

My experiments confirm that indeed this PR surfaced the bug in process currently affecting GHCs 9.2.8, 9.4.7, and 9.6.3. Sorry, you stepped on a mine here, you take no blame.

JobPetrovcic pushed a commit to JobPetrovcic/agda that referenced this pull request Apr 12, 2024
Instead of manipulating the system environment, just pass an updated environment when you run the agda process.
This way, environment settings from one test case cannot leak into another one in concurrent execution.

Commits:

* Set subprocess env declaratively

* Add back env-var expansion

* Use reverse instead of foldl'

* Calculate agda env-args without IO

* Add type synonym

* Purify getAgdaBin

* Fix confusion of extra/combined env

* Refactor fold

* Document .flags and .vars testing infrastructure

* Fix whitespace

* Update comment

* Cosmetic fixes in `.vars` description

* Apply suggestions from code review

Co-authored-by: Andreas Abel <andreas.abel@ifi.lmu.de>

---------

Co-authored-by: Andreas Abel <andreas.abel@ifi.lmu.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra: test suite Issues relating to the test suite (not in changelog) pr: squash-me This PR needs squashing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DuplicateExecutable test randomly breaks other Fail/ tests
2 participants