Skip to content

Fix Cygwin ports#2266

Merged
shindere merged 1 commit intoocaml:trunkfrom
dra27:fix-cygwin-exe
Feb 28, 2019
Merged

Fix Cygwin ports#2266
shindere merged 1 commit intoocaml:trunkfrom
dra27:fix-cygwin-exe

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 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
Copy link
Copy Markdown
Member Author

dra27 commented Feb 26, 2019

Precheck job 195

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Feb 26, 2019 via email

@dra27
Copy link
Copy Markdown
Member Author

dra27 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
Copy link
Copy Markdown
Member

gasche 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
Copy link
Copy Markdown
Member Author

dra27 commented Feb 27, 2019

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

Copy link
Copy Markdown
Member

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

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

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

@shindere
Copy link
Copy Markdown
Contributor

@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!

@shindere shindere merged commit 899b48a into ocaml:trunk Feb 28, 2019
shindere pushed a commit that referenced this pull request Feb 28, 2019
@shindere
Copy link
Copy Markdown
Contributor

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

@dra27 dra27 deleted the fix-cygwin-exe branch July 6, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants