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

Fix Cygwin ports #2266

Merged
merged 1 commit into from Feb 28, 2019

Conversation

Projects
None yet
4 participants
@dra27
Copy link
Contributor

commented Feb 26, 2019

Cygwin should have EXE=.exe in Makefile.config (it did with the old configure system).

This causes a strange chain of events leading to a broken compiler: because of how Cygwin handles .exe, the camlheader programs end up with a .exe extension (NB this is Cygwin only - the native Windows builds remain correct, with no .exe extension on the camlheader programs). This causes a problem with #2041 since Sys.file_exists "camlheader" will be true on Cygwin if camlheader.exe exists and is execuable but Sys.readdir will not contain camlheader and so the programs built by the Cygwin builds of ocamlc will be headerless.

This is manifesting itself in testsuite failures - it obviously doesn't affect the build, because bytecode images are never executed directly.

cc @shindere

@dra27 dra27 added the bug label Feb 26, 2019

@dra27 dra27 added this to the 4.08 milestone Feb 26, 2019

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

No problem, @shindere! I don't think parallel builds worked for Cygwin before all the build system changes, either (and they definitely didn't for the native Windows build system!).

For the Changes, I was doing this as we did with GPR#1200 (which was the Unicode PR for Windows) where any additional PRs during the release cycle were simply added to the list and then after 4.06.0 was released any further fixes got put on their own (e.g. GPR#1629 in 4.06.1). Any comment, @gasche, as keeper of Changes?

This certainly does need to go in 4.08, yes - ocamlc is basically broken on Cygwin without this change.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Re. Changes: I think that either approaches are reasonable, but I think @shindere's point was also that debugging this one issue was particularly delicate, and handling it separately better credits the effort evolved. (If I understand correctly, Jérémie was also involved in tracking it down.) I tend to agree with this idea of separating the most work-inducing changes on their own.

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

OK - Changes entry moved. Those two commits can be squashed onto trunk and then cherry-picked to 4.08.

@damiendoligez
Copy link
Member

left a comment

This patch is correct and must be cherry-picked to 4.08.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@dra27 CI is okay but there are conflicts in Changes.

Would you mind doing the rebase, conflict resolution and squashing?

Then I'd be happy to merge, unless you prefer to do it yourself at the same
time!

@dra27 dra27 force-pushed the dra27:fix-cygwin-exe branch from d0eeaff to 6e7c43c Feb 28, 2019

@shindere shindere merged commit 899b48a into ocaml:trunk Feb 28, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

shindere added a commit that referenced this pull request Feb 28, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Merged into trunk and cherry-picked to 4.08 as commit
2c712cd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.