-
Notifications
You must be signed in to change notification settings - Fork 707
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
Monitor also ghc.shim file on Windows #10853
base: master
Are you sure you want to change the base?
Monitor also ghc.shim file on Windows #10853
Conversation
4045609
to
ebad40d
Compare
I think adding a test that tests multiple GHC versions changing is out of the capabilities of the test setup, so I ask for permission to merge this without a test. |
ebad40d
to
e5e5884
Compare
To be honest... this is a huge hack. Cabal shouldn't be required to know about this. It's an implementation detail of GHCup. The culprit is that windows doesn't have reliable symbolic link support. What cabal should really care about is the GHC version or even ABI. Can we not run |
It is indeed a big hack. I am open to suggestions. I think changing the whole mechanism for detecting changes from files changing to output of commands sounds like a big task, but it might indeed be the right way of solving this. |
@@ -989,6 +989,9 @@ programsMonitorFiles progdb = | |||
monitorFileSearchPath | |||
(programMonitorFiles prog) | |||
(programPath prog) | |||
++ if programId prog == "ghc" && buildOS == Windows | |||
then [monitorFile $ programPath prog -<.> "shim"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the file doesn't exist?
@jasagredo Considering the tricky nature of this PR, I think it would have been better to discuss it in a ticket beforehand and gather feedback from all parties. |
@jasagredo I do agree this is a bit hacky but it might be the best thing to do. If you want to implement it like this, the correct way to do so would be to add this path to The other alternative, is to modify the caching logic for this step. Firstly you would need to split up the
How does that sound to you? |
@Kleidukos Feel free to continue the discussion in the linked issue. I just thought it would be better for discussion to have the proposed change somewhere as it is barely 3 lines of code. |
The way -- | Creates a list of files to monitor when you search for a file which
-- unsuccessfully looked in @notFoundAtPaths@ before finding it at
-- @foundAtPath@.
monitorFileSearchPath :: [FilePath] -> FilePath -> [MonitorFilePath]
monitorFileSearchPath notFoundAtPaths foundAtPath =
monitorFile foundAtPath
: map monitorNonExistentFile notFoundAtPaths So we would have to use some other new field for this. |
An unrelated note: |
On Linux, when changing the compiler cabal says:
On Windows after this fix it will say:
which actually is the file that changes when GHCup switches versions.
QA
Template Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
Is the change significant? If so, remember to addsignificance: significant
in the changelog file.The documentation has been updated, if necessary.Tests have been added.(Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)