-
Notifications
You must be signed in to change notification settings - Fork 712
Windows: rewrite paths to configure #7652
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
Conversation
I don't trust my understanding of this toolset any more, but it generally looks good. Let's remember to revert the failed fix first (#7651) and then this PR can be a tiny bit smaller. |
98d1120
to
725944b
Compare
So the idea is to convert |
I've merged #7651 that reverts the old fix, hence the conflict, but the conflicting bit can just be removed, it's already applied. |
725944b
to
9e3d558
Compare
Oh, I missed that |
Ah, that's why! lol |
9e3d558
to
1764980
Compare
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.
Looks good, but we need tests with scripts generated with various versions of autoconf to make sure the layers of hacks around all this don't intervene.
I've tested it in my windows 10:
previously i checked the same steps with the cabal-3.6.0.0 release version and it failed Otoh the build generating the config files with autoconf-2.71 in windows also works:
|
|
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.
LGTM, thanks @Mistuke for taking care
@Mistuke: we are waiting until you have some breathing space to have a slow, careful, final look before this is merged. No pressure, of course, the whole world can wait. ;D I mean, this problem evaded quite extensive testing last time, so we are not as keen as before to believe the tests this time. Hence the special weight of your expertise. |
I've merged #7653 with extra checks that may be useful to test this PR, so let me rebase and see. |
@Mergifyio rebase |
Command
|
1764980
to
320d0bf
Compare
Morning! I have some work to finish this morning but will do some manual checks today after work in ~9h. |
@jneira thanks for also testing so far. |
@Mistuke |
@jneira those files belong to automake, autoconf only reads them not makes them. If you really want to generate new ones you should use |
It looks like only the path to configure is being rewritten as expected:
so that's good. Building both my local and package network works:
Looking at the log file:
and nothing else uses the new paths, Everything else still uses |
Thanks, i might overlook it remembering this issue: #7170 |
That's unrelated to this. There's no configuring going on there and the post contains too few information to tell what's going on. Also these things come with a significant performance cost (which is why I and a lot of people have them disabled everywhere):
|
Thanks for share your knowledge and your patience 🙂 |
That issue just looks like either cabal is not given the short pathname at all by the shell or something is expanding it to the full path. I'll be honest, not really sure it's worth fixing. I think focus should be to just enable winio and move on. Dos shortnames are legacy and not supported by newer windows filesystems anyway. |
CI on master is now fixed, so let me rebase. |
@Mergifyio rebase |
Command
|
320d0bf
to
5da51fc
Compare
Oh, dear, I forgot the PR that fixes the last bit of master CI doesn't have enough reviews and so is not merged. So, it's going to fail. |
#7659 is merged so rebasing |
Command
|
5da51fc
to
4c800c9
Compare
@Mergifyio backport 3.6 |
Command
|
Well done. Full green CI, finally, including the dogfooding tests that catch the original bug in cabal 3.6 (though not the new autoconf problems that were present in 3.4, because the new autoconf configs are not yet present in packages on Hackage, I think). |
As soon as #7664 gets merged to 3.6 by mergifyIO, we can backport this again to 3.6 and it should merge correctly and pass CI fine this time. This will add the last missing piece in the back/forward-porting effort related to 3.6.2 and CI failures:
|
Wow, many thanks for this tour the force and share the info |
I'm not working today, but let me just repeat what @jneira did 18 hours ago and for what 3.6 is now finally ready: |
Command
|
Huh, actually mergifyIO mentions the same old PR instead of creating a new one. So I guess, this needs to be backported manually after all. See you on Monday! |
Windows: rewrite paths to configure (backport #7652)
Should fix #7649 but I don't have msys2 on this machine so other than building Cabal I can't test until I get home.
@jneira @Mikolaj , @jneira could you test this until then?
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!