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 the -runtime-variant option for bytecode #8537

Merged
merged 4 commits into from Jun 11, 2019

Conversation

@damiendoligez
Copy link
Member

commented Mar 22, 2019

The -runtime-variant option has been broken since 4.03.0 because the camlheaderd and camlheaderi files are installed under the wrong names. The first commit fixes this problem.

The failure is silent and a bytecode file is produced that has the executable flag but doesn't work because the header is missing. The second commit adds some error handling for this case (in particular when the user specifies a runtime variant that doesn't exist).

Note: the first bug was introduced by myself in commit 0225ca0

@damiendoligez damiendoligez force-pushed the damiendoligez:runtime-variant-fixes branch 2 times, most recently from 6deb0af to a8b0952 Mar 22, 2019
@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

See also #2309 and #6199.

Copy link
Contributor

left a comment

Also related to #2267. I appear to be in a very nitty mood this morning with the other comments.

Changes Outdated Show resolved Hide resolved
@@ -695,7 +698,9 @@ let report_error ppf = function
fprintf ppf "Error on dynamically loaded library: %a"
Location.print_filename file
| Required_module_unavailable s ->
fprintf ppf "Required module `%s' is unavailable" s
fprintf ppf "Required module '%s' is unavailable" s

This comment has been minimized.

Copy link
@dra27

dra27 Mar 26, 2019

Contributor

Why the elimination of the back-tick here? FWIW, I'd prefer to see other instances of '%s' changed to `%s' rather than this way around?

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Mar 26, 2019

Contributor

@dra27 some people from your faculty may disagree ;-)

This comment has been minimized.

Copy link
@dra27

dra27 Mar 26, 2019

Contributor

Quite possibly - but let's remove them wholesale, rather than piecemeal with other changes 🙂

This comment has been minimized.

Copy link
@dra27

dra27 Mar 26, 2019

Contributor

(I'd also prefer them either to be post-processed to the correct Unicode character before going to the console, or for us finally to allow the correct Unicode characters directly in the message, both of which would keep Markus happy!)

bytecomp/bytelink.ml Outdated Show resolved Hide resolved
@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I have a nagging concern that the flexlink bootstrap relies on the "silent fail" behaviour when the header is missing - I just want to check what's going on there before this is merged (I'm suspicious that in bytecode-only mode, it may result in a corrupt flexlink.exe being installed).

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Just to confirm - this PR will break the flexdll bootstrap build the next time someone commits a bootstrap. Do you mind if I push fixes directly?

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I've rebased and added commits to fix the flexdll bootstrap in my fork

@dra27 dra27 referenced this pull request Mar 28, 2019
@damiendoligez damiendoligez force-pushed the damiendoligez:runtime-variant-fixes branch 2 times, most recently from 2d7eef4 to 98c598d May 9, 2019
@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

@dra27 I've applied all your suggestions and integrated your flexdll fixes (tested by bootstrapping flexdll on mingw64).

I've also moved the Change entry to 4.09, as I intend to cherry-pick this patch.

@dra27
dra27 approved these changes Jun 10, 2019
@dra27

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@damiendoligez - squash or merge, and still good for 4.09?

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

This is a bugfix so it should be good for 4.09.
(I have a marked preference for having merge commits. In this case the history is not great, but we don't want an extra roundtrip with the contributor, and it's not so bad, so I would just Merge directly.)

damiendoligez and others added 4 commits Mar 22, 2019
… under

the wrong names, which made -runtime-variant fail for bytecode.
If OCaml was already installed, camlheader would be used from the
installation when building the bytecode flexdll/flexlink.exe
@dra27 dra27 force-pushed the damiendoligez:runtime-variant-fixes branch from 98c598d to 5231ae1 Jun 10, 2019
@dra27

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

FWIW, I just squashed the Changes bit - the flexdll and other changes are a bit subtle, so seem worth retaining cleanly for bisecting

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

I'll merge and cherry-pick once CI passes

@dra27 dra27 merged commit 8153041 into ocaml:trunk Jun 11, 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
dra27 added a commit that referenced this pull request Jun 11, 2019
fix the -runtime-variant option for bytecode

(cherry picked from commit 8153041)
@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Thanks!

@damiendoligez damiendoligez deleted the damiendoligez:runtime-variant-fixes branch Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.