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

Remove --cabal-file option #9123

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

andreabedini
Copy link
Collaborator

Closes: #8395


Please read Github PR Conventions and then fill in one of these two templates.


Template Α: This PR modifies cabal behaviour

Include the following checklist in your PR:

Bonus points for added automated tests!


Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

Include the following checklist in your PR:

@andreabedini andreabedini self-assigned this Jul 14, 2023
@andreabedini andreabedini force-pushed the andrea/remove-cabal-file branch from 1ea8a0d to 3c2c6b8 Compare July 17, 2023 07:11
@andreabedini
Copy link
Collaborator Author

-Documentation created: ../setup-external.dist/work/mylib/dist/doc/html/mylib/index.html
+Documentation created: ../setup-external.dist/work/mylib/dist/doc/html/mylib/

I am running into this annoying problem 🤔 😒

@ulysses4ever
Copy link
Collaborator

I am running into this annoying problem thinking unamused

It was recently changed in #8919 (see how it updated the tests). Not sure what to do about it though.

@ulysses4ever
Copy link
Collaborator

We could tag the author of that PR maybe.

@andreabedini
Copy link
Collaborator Author

What's puzzling is that it seems to depend on the GHC version 🤔 Let me reset those changes first.

@andreabedini andreabedini force-pushed the andrea/remove-cabal-file branch from 3c2c6b8 to 362a714 Compare July 17, 2023 16:18
@ulysses4ever
Copy link
Collaborator

It may have to do with the version of Haddock (distributed with GHC, so, by extension, on the version of GHC). But yeah, I have no idea really.

@gbaz gbaz changed the title Remove cabal file Remove --cabal-file option Jul 17, 2023
@andreabedini andreabedini force-pushed the andrea/remove-cabal-file branch from 362a714 to 898d94b Compare July 18, 2023 06:27
@andreabedini
Copy link
Collaborator Author

Maybe I was just mixing up two different failures.

@andreabedini
Copy link
Collaborator Author

Can any Windows expert help me understand what's going on here https://github.com/haskell/cabal/actions/runs/5584068207/jobs/10205063977#step:18:1658?

@andreabedini andreabedini added the attention: needs-help Help wanted with this issue/PR label Jul 18, 2023
@gbaz
Copy link
Collaborator

gbaz commented Jul 18, 2023

might be sporadic, triggered a rerun

@Mikolaj
Copy link
Member

Mikolaj commented Sep 12, 2023

Let me try to rerun once again. :)

@andreabedini
Copy link
Collaborator Author

Thanks. I am at loss with what is happening on Windows :-/ I don't think I can do much.

@andreabedini andreabedini marked this pull request as ready for review October 11, 2023 09:25
@jasagredo
Copy link
Collaborator

jasagredo commented Nov 23, 2023

I'm able to reproduce the error in 9.4.4:

Running: "C:\ghcup\ghc\9.4.4\lib\../mingw/bin\llvm-ar.exe" "-qL" "..\setup-internal.dist\work\Includes2\dist\build\Includes2-0.1.0.0-mylib+JA8sqakdQMC7oNcBYWFaHI\objs-7580\libHSIncludes2-0.1.0.0-mylib+JA8sqakdQMC7oNcBYWFaHI.a" "@..\setup-internal.dist\work\Includes2\dist\build\Includes2-0.1.0.0-mylib+JA8sqakdQMC7oNcBYWFaHI\objs-7580\ar58B.rsp"
C:\ghcup\ghc\9.4.4\lib\../mingw/bin/llvm-ar.exe: warning: creating ..\setup-internal.dist\work\Includes2\dist\build\Includes2-0.1.0.0-mylib+JA8sqakdQMC7oNcBYWFaHI\objs-7580\libHSIncludes2-0.1.0.0-mylib+JA8sqakdQMC7oNcBYWFaHI.a
C:\ghcup\ghc\9.4.4\lib\../mingw/bin/llvm-ar.exe: error: ..\setup-internal.dist\work\Includes2\dist\build\Includes2-0.1.0.0-mylib+JA8sqakdQMC7oNcBYWFaHI\objs-7580\libHSIncludes2-0.1.0.0-mylib+JA8sqakdQMC7oNcBYWFaHI.a: No such file or directory

It seems the folder doesn't exist. I can confirm that llvm-ar.exe cannot create directories:

Javier@DESKTOP-JDT20GG MINGW64 ~/code/cabal
$ /c/ghcup/ghc/9.4.4/mingw/bin/llvm-ar qL xx/a.a
C:\ghcup\ghc\9.4.4\mingw\bin/llvm-ar.exe: warning: creating xx/a.a
C:\ghcup\ghc\9.4.4\mingw\bin/llvm-ar.exe: error: xx/a.a: No such file or directory

Javier@DESKTOP-JDT20GG MINGW64 ~/code/cabal
$ mkdir xx

Javier@DESKTOP-JDT20GG MINGW64 ~/code/cabal
$ /c/ghcup/ghc/9.4.4/mingw/bin/llvm-ar qL xx/a.a
C:\ghcup\ghc\9.4.4\mingw\bin/llvm-ar.exe: warning: creating xx/a.a

Javier@DESKTOP-JDT20GG MINGW64 ~/code/cabal
$ ls xx
a.a

However, the test passes with GHC 9.6.3, and the behavior of llvm-ar.exe is the same:

Javier@DESKTOP-JDT20GG MINGW64 ~/code/cabal
$ /c/ghcup/ghc/9.6.3/mingw/bin/llvm-ar qL yy/a.a
C:/ghcup/ghc/9.6.3/mingw/bin/llvm-ar.exe: warning: creating yy/a.a
C:/ghcup/ghc/9.6.3/mingw/bin/llvm-ar.exe: error: yy/a.a: No such file or directory

Which makes me think that GHC changed something in the steps it followed for creating the paths.

Confirmed it works also on GHC-9.6.1, so the failure is on 9.4.4

@jasagredo
Copy link
Collaborator

jasagredo commented Nov 23, 2023

Note that MinGW does not understand symlinks, so the test would need to be reworked around this fact (or skipped in windows)

What I mean by this is that the symlinks you have in the Includes2 folder will not work on MinGW on Windows. I had to manually replace them with actual copies of the directories for the tests to run. I don't understand how they can run in GHA (perhaps Git bash?)

@jasagredo
Copy link
Collaborator

jasagredo commented Nov 23, 2023

So in fact it has nothing to do with directories but rather with long paths, and it was fixed in 9.4.5:

$ /c/ghcup/ghc/9.4.5/mingw/bin/llvm-ar --version
LLVM (http://llvm.org/):
  LLVM version 14.0.6
  Optimized build.
  Default target: x86_64-w64-windows-gnu
  Host CPU: znver3

Rebasing on top of master should fix this

To confirm this, I shortened the length of the package and sub libraries to 1 letter only and then progressively add letters to --builddir and indeed it worked until I reached abc...m 😁

@andreabedini andreabedini force-pushed the andrea/remove-cabal-file branch from db857ee to cd83aee Compare November 24, 2023 11:38
@andreabedini
Copy link
Collaborator Author

And indeed it works! Thank you again ❤️

@andreabedini andreabedini added attention: needs-review and removed attention: needs-help Help wanted with this issue/PR labels Nov 25, 2023
@andreabedini
Copy link
Collaborator Author

@ulysses4ever @Mikolaj gentle ping for a review ☺️

@Mikolaj
Copy link
Member

Mikolaj commented Dec 1, 2023

Thanks a lot for the tests! How about a changelog?

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

LGTM, thank you. And sorry for long delay.

The flag is still available in the library and perhaps v1-commands? There're setup-tests that you updated to not use the flag, but is it really unavailable in setup-tests or is it out of a desire to avoid the legacy flag?

@andreabedini
Copy link
Collaborator Author

The flag is still available in the library and perhaps v1-commands?

It is still available in v1-configure and in ./Setup.hs

~/code/cabal andrea/remove-cabal-file
❯ cabal run -- cabal v1-configure --help | grep cabal-file
 --cabal-file=PATH              use this Cabal file

~/code/cabal andrea/remove-cabal-file
❯ cabal run -- cabal act-as-setup --build-type Simple -- configure --help | grep cabal-file
Warning: this is a debug build of cabal-install with assertions enabled.
 --cabal-file=PATH              use this Cabal file

There're setup-tests that you updated to not use the flag, but is it really unavailable in setup-tests or is it out of a desire to avoid the legacy flag?

🤔 uhm, TBH, I don't remember what I was thinking 5 months ago 😂 yes, perhaps I could have kept the tests the same, but if there is a way to run the tests without "internal" options perhaps that is the better way to go.

@ulysses4ever
Copy link
Collaborator

That is fair and I agree. Is there no place in the docs to expand a little bit on the "internal" nature of this flag? I remember users trying to use the flag in all sorts of wrong ways...

@ulysses4ever
Copy link
Collaborator

And not only that but just improve its docs.

@andreabedini
Copy link
Collaborator Author

That is fair and I agree. Is there no place in the docs to expand a little bit on the "internal" nature of this flag? I remember users trying to use the flag in all sorts of wrong ways...

There is space but I am afraid someone else will have to do this. I don't know much about that flag; only from reading #8395 I thought it was a low-hanging fruit ☺️ and here we are half a year later 😂

@Mikolaj
Copy link
Member

Mikolaj commented Dec 6, 2023

That is fair and I agree. Is there no place in the docs to expand a little bit on the "internal" nature of this flag? I remember users trying to use the flag in all sorts of wrong ways...

There is space but I am afraid someone else will have to do this. I don't know much about that flag; only from reading #8395 I thought it was a low-hanging fruit ☺️ and here we are half a year later 😂

Could you open a short ticket about it and mark it documentation? Thank you.

I still don't see a changelog file. Am I missing it?

@Mikolaj
Copy link
Member

Mikolaj commented Dec 6, 2023

Alternatively, perhaps let's not close #8395 and let's add the note about the docs there.

@andreabedini andreabedini force-pushed the andrea/remove-cabal-file branch from cd83aee to 5557530 Compare December 6, 2023 15:08
@andreabedini
Copy link
Collaborator Author

@Mikolaj I have added the changelog ;-)

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.

This looks fine. Thank you.

@andreabedini andreabedini added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Dec 7, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Dec 9, 2023
@Mikolaj Mikolaj force-pushed the andrea/remove-cabal-file branch from 5557530 to cd7b3d6 Compare December 9, 2023 02:30
@mergify mergify bot merged commit eeb99c9 into haskell:master Dec 9, 2023
49 checks passed
@andreabedini andreabedini deleted the andrea/remove-cabal-file branch December 12, 2023 04:47
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 merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--cabal-file needs to go
5 participants