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 parallel build on Cygwin #2267

Merged
merged 1 commit into from Apr 9, 2019

Conversation

@dra27
Copy link
Contributor

commented Feb 26, 2019

The Unix and Cygwin build instructions in stdlib/Makefile are incorrect. They appear to have been written on the assumption that:

foo bar: baz
	command

means that foo and bar are generated by a single command, which is incorrect as that really defines two separate recipies. On Unix, with #! scripts, this just means that the headers are generated more times than necessary but for Cygwin it means that the for loop which was there is called multiple times and unsurprisingly in parallel those calls interfere with each other.

Unusually, the native Windows build instructions were better, so my approach has been to merge them both (there was a TODO note to this effect already)

I have also fixed a note that the build should not be done using flexlink - this has been done by formally promoting MKEXE_BOOT to the entire build system (it means "do MKEXE, but don't use flexlink"). At some point I may come up with a better name, I had simply started this fix until I realised that #2266 was the minimum 4.08 required fix and the lack of parallelism on Cygwin is annoying (at least, possibly uniquely, for me...)

The diff is very awkward to read - I'd recommend simply reviewing my resulting code in stdlib/Makefile as though there were nothing there before.

Windows and Unix build instructions for the program versions of the
header stubs unified. For Cygwin, this also fixes the parallel build.
@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

Thanks, @shindere! For the pattern rules comment, I was trying to explain that the more natural-looking:

camlheader%: header%.$(O)
	...

works for camlheaderd and camlheaderi but doesn't work for camlheader - i.e. the % in a pattern must have something to match so I do camlhead%: and know that the pattern will always include er even in camlheader but it involves the ugly $(subst er,,$*) later and the even stranger looking use of the . in .exe in the tmpheader files!

I wasn't planning on dealing with the FIXME now - it's been like that for decades, so it can wait for a bit longer! It will almost certainly be picked up when cross-compilation is being done, because strip is invoked without a prefix, so the tool name should be being picked up in configure (I expect that's also the case elsewhere in the build system, which is another reason I elected to leave it be for now)

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

@shindere - I reworded the comment in stdlib/Makefile which I hope is clearer (my main concern was to ensure that no one is tempted to correct what initially looks like a typo, although anyone doing that would quickly be presented with a failing build!).

I have changed enough (not building flexlink, moving MKEXE_BOOT, etc.) that I don't propose this for 4.08. We could possibly choose to include it in a 4.08.1, just for the convenience of having parallel builds.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@dra27 dra27 force-pushed the dra27:merge-headers branch from f76dc16 to bb9a486 Feb 28, 2019
@trefis

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Ping: is this PR ready to be approved or does more work need to happen?

@dra27 dra27 force-pushed the dra27:merge-headers branch from bb9a486 to cf4221e Apr 9, 2019
@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Rebased and ready to go as far as I'm concerned, but note (again) that this is not for 4.08, at this stage.

@shindere shindere merged commit 278e5ab into ocaml:trunk Apr 9, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shindere

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

This PR has introduced the TARGETHEADERPROGRAM variable which is used in
stdlib/Makefile but, I think, never defined.

Under some circumstances (hashbang script support not available and
stdlib/header.c and stdlib/headernt.c more recent than stdlib/.depend,
this causes the build system of the standard library to be broken.

#8626 has just been submitted to fix this.

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