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.

Sign up for GitHub

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

Jump to bottom

test/Succeed/ForeignPragma has CI glitches #6739

Closed
andreasabel opened this issue Jul 24, 2023 · 3 comments · Fixed by #7264
Closed

test/Succeed/ForeignPragma has CI glitches #6739

andreasabel opened this issue Jul 24, 2023 · 3 comments · Fixed by #7264
Assignees
@andreasabel
Labels
infra: github workflows Issues related to GitHub workflows and actions (not in changelog) infra: test suite Issues relating to the test suite (not in changelog)
Milestone
2.6.4

Comments

@andreasabel
Copy link
Member

andreasabel commented Jul 24, 2023 •
edited

https://github.com/agda/agda/actions/runs/5645321781/job/15291549031?pr=6738#step:10:38

out > Compilation error:
out >
out > <no location info>: error:
out >     renameFile:renamePath:rename MAlonzo/RTE.o.tmp' to MAlonzo/RTE.o': does not exist (No such file or directory)

Also:
@andreasabel andreasabel added infra: test suite Issues relating to the test suite (not in changelog) infra: github workflows Issues related to GitHub workflows and actions (not in changelog) labels Jul 24, 2023
@andreasabel
Copy link
Member Author

andreasabel commented Jul 24, 2023 •
edited

Maybe GHC chokes when it is operating concurrently on the same files. I found a function that could be involved in the case:
https://gitlab.haskell.org/ghc/ghc/-/blob/73b5c7ce33929e1f7c9283ed7c2860aa40f6d0ec/compiler/GHC/Utils/Misc.hs#L1265-1275

withAtomicRename :: (MonadIO m) => FilePath -> (FilePath -> m a) -> m a
withAtomicRename targetFile f = do
  -- The temp file must be on the same file system (mount) as the target file
  -- to result in an atomic move on most platforms.
  -- The standard way to ensure that is to place it into the same directory.
  -- This can still be fooled when somebody mounts a different file system
  -- at just the right time, but that is not a case we aim to cover here.
  let temp = targetFile <.> "tmp"
  res <- f temp
  liftIO $ renameFile temp targetFile
  return res

The name says "atomic", but nothing is atomic here. GHCs running in parallel could both try to renameFile, the first one succeeds, the second one crashes.

I filed a question to GHC:

andreasabel added a commit that referenced this issue Jul 24, 2023
@andreasabel
CI: partial workaround #6739: run tests even if other tests failed
27e14dc
@andreasabel andreasabel mentioned this issue Jul 24, 2023
Merged
andreasabel added a commit that referenced this issue Jul 24, 2023
@andreasabel
CI: partial workaround #6739: run tests even if other tests failed
ecce22c
andreasabel added a commit that referenced this issue Jul 25, 2023
@andreasabel
Re #6739: future-proof check for -c/--compile/--ghc in test runner
42b1abc 
Maybe a cause for #6739 was that the check was for `--compile` only
but the `.flags` files in `test/Succeed` also used `-c`.
This commit updates the check and prevents it from bitrotting.
andreasabel added a commit that referenced this issue Jul 25, 2023
@andreasabel
Re #6739: some testsuite debug printing (commented out)
ec39a17
@andreasabel andreasabel mentioned this issue Jul 25, 2023
Merged
@andreasabel
Copy link
Member Author

The GHC developers answered that our concurrent use of GHC on the same file is problematic.

Maybe such concurrent use was never intended by the testsuite and just crept in there: in that case, it could be fixed by

Optimistically closing...

@andreasabel andreasabel added this to the 2.6.4 milestone Jul 25, 2023
@andreasabel andreasabel self-assigned this Jul 25, 2023
@andreasabel andreasabel mentioned this issue Feb 7, 2024
Closed
JobPetrovcic pushed a commit to JobPetrovcic/agda that referenced this issue Apr 12, 2024
@andreasabel @JobPetrovcic
CI: partial workaround agda#6739: run tests even if other tests failed
b5b2371
JobPetrovcic pushed a commit to JobPetrovcic/agda that referenced this issue Apr 12, 2024
@andreasabel @JobPetrovcic
Re agda#6739: future-proof check for -c/--compile/--ghc in test runner
24cd963 
Maybe a cause for agda#6739 was that the check was for `--compile` only
but the `.flags` files in `test/Succeed` also used `-c`.
This commit updates the check and prevents it from bitrotting.
JobPetrovcic pushed a commit to JobPetrovcic/agda that referenced this issue Apr 12, 2024
@andreasabel @JobPetrovcic
Re agda#6739: some testsuite debug printing (commented out)
673310b
@andreasabel
Copy link
Member Author

This glitch is still existing for other testcases: https://github.com/agda/agda/actions/runs/9093876160/job/24994686520#step:10:36

@andreasabel andreasabel reopened this May 15, 2024
andreasabel added a commit that referenced this issue May 15, 2024
@andreasabel
Fix #6739: testing GHC backend: use separate -outputdir to avoid races
5b80780 
Hopefully fixes #6739 (time will tell as races only show up sometimes).
@andreasabel andreasabel mentioned this issue May 15, 2024
Merged
andreasabel added a commit that referenced this issue May 15, 2024
@andreasabel
Fix #6739: testing GHC backend: use separate -outputdir to avoid races
c78bfb0 
Hopefully fixes #6739 (time will tell as races only show up sometimes).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andreasabel andreasabel

Labels
infra: github workflows Issues related to GitHub workflows and actions (not in changelog) infra: test suite Issues relating to the test suite (not in changelog)
Projects
None yet
Milestone
2.6.4
Development

Successfully merging a pull request may close this issue.

Fix #6739: testing GHC backend: use separate -outputdir to avoid races agda/agda
1 participant
@andreasabel