-
Notifications
You must be signed in to change notification settings - Fork 701
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
Set PATH_SEPARATOR=; when calling ./configure; fix #7494 #7510
Conversation
so we should test it locally I suppose, will do asap |
@jneira: thank you. Please try with the second commit to make sure my CPP works fine. |
nice, I already reproduced the error in my windows 10 just in case. |
Perhaps old-time? Just guessing. No other mentions in the issue. |
Also https://hackage-search.serokell.io/?q=build-type%3A.*Configure |
changelog.d/pr-7510
Outdated
|
||
description: { | ||
|
||
- Set PATH_SEPARATOR=";" for ./configure on Windows; fix for new autoconf |
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.
- Set PATH_SEPARATOR=";" for ./configure on Windows; fix for new autoconf | |
- Set PATH_SEPARATOR=";" for ./configure on Windows; fix for autoconf >= 2.70 |
(it can also just go all in the synopsis with no extended description)
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.
Heh, the details didn't fit in 80 lines, despite condensing wrt synposis. Let me go full hog in synopsis, after your encouragement.
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.
Oh, there's no hard limit on synopsis. It's just converted into changelog bullet points (or sub-points for description) after all
😮 i did not know it, pure gold, thanks! |
I will try another pair of projects |
I've tried with process and both versions of cabal works as intended. I think the cause is for process there are no |
What about the TODO? |
@fgaz: this TODO is a style thing, and I still don't quite feel the style of this codebase, so I'd need a guidance. What would be your recommendation? Keep the sole CPP (ATM it doesn't look like it could grow or multiply) or create a value in a Compat module and, if so, in which one? |
I did not test it with an older autoconf version, I have to downgrade it manually to do it |
The build ends with some warnings in all cases so i guess they are fine |
I tested time with the same results |
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, all tests are ok I my end
Yet another random fail from AppVeyor. I will rebase the branch and this will reset the tests and let's hope they pass. |
@Mergifyio backport 3.6 |
Command
|
Revert PR #7510 (which was "Set PATH_SEPARATOR=; when calling ./configure")
No tests, because they depend on a particular version of autoconf and I think the current tests of autoconf would already fail with the new autoconf version on Windows CI (but either they are not run on Windows or autoconf on CI is old; they are run on Linux, because setting PATH_SEPARATOR breaks these tests and @jneira kindly reproduced the fail on Windows locally).
Edit: what I missed here and what led to regression in cabal 3.6 is that packages can carry their own generated autoconf files, from versions we have no control of (e.g., which did not run with "PATH_SEPARATOR=;" nor did predict such usage, but now we are using them with "PATH_SEPARATOR=;"). And I bet the story is even more complex.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!