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 assembler detection in configure #2311

Merged
merged 3 commits into from Mar 13, 2019

Conversation

Projects
None yet
3 participants
@shindere
Copy link
Contributor

commented Mar 12, 2019

This PR fixes the detection of the assembler program on some platforms,
amnong which Mac OS. It thus fixes MPR#7919.

The PR also makes sure the CPP build variable is dealt with properly,
which should also be helpful for GPR##2295

See precheck build #216(https://ci.inria.fr/ocaml/job/precheck/216).

(Should be cherry-picked to 4.08.)

@dra27
Copy link
Contributor

left a comment

Tweak needed for MSVC - I'm not sure how important it actually is, but it seems sensible to maintain the previous value of CPP on the two MSVC ports.

Show resolved Hide resolved configure.ac
Show resolved Hide resolved configure.ac Outdated

shindere added some commits Mar 12, 2019

Clarify the invocation of the C preprocessor
Most of the time, the C preprocessor needs to be invoked through the C
compiler, e.g. so that the paths to the header files are resolved properly.
In some cases, though, we really need to be able to call the C
preprocessor directly, just to expand macros in .ml files (this only
happens in the testsuite, at the moment). In those cases, it is
simply impossible to call the C preprocessor through the compiler,
e.g. because this would require the input files to have a '.c'
extension, which the OCaml compiler would misinterprete as meaning this file
should be compiled with the C compiler.

Thus, this commit clarifies the distinction between CPP and DIRECT_CPP
and provides both variables to the build system. The ocamltest build system
is also updated to take advantage of this.

We rely on autoconf's macros to detect how to call the C preprocessor
via the C compiler, except for the MSVC port where its value is hard-coded
to guarantee backward compatibility.

@shindere shindere force-pushed the shindere:fix-configure branch from fe56392 to 3c2c0eb Mar 12, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@shindere wrote:

Could you please have a look at how I modified the code and tell me what
you think about this new version? For the moment I kept the call to
AC_SUBST because it felt more explicit to me, what do you think?

I was suggesting to keep the AC_SUBST, just to put it in a different place - what "worries" me slightly is that I saw it and thought "it's not necessary, that variable's initialised with AC_CHECK_TOOL", but that's not actually true for MSVC. So by having AC_SUBST([DIRECT_CPP],[$CPP]) later, that felt to me more explicit.

@ksromanov

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

I rebased changes in GPR #2295 on top of this branch - everything works exceptionally well!

Tested systems (compile, run repl, #load "unix.cma"):

  1. xlc/AIX/POWER/32-bit
  2. xlc/AIX/POWER/64-bit
  3. gcc4.3/AIX/POWER/32-bit
  4. gcc5.3/linux/x86-32
@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@shindere - for the AC_SUBST change, that's up-to-you! I've stated what worries me about how it is, which you're very welcome to either agree with and change to, or not share my worry :) I don't insist, no - especially as I can see why you grouped all the AC_SUBST lines up there in the first place.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

There will presumably be another beta before 4.08 goes out of the door, @damiendoligez ... minor though the change hopefully is, the effective change to utils/config.ml by going with autoconf's CPP value I don't think should appear between beta and release?

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Other than that, LGTM.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@shindere - re 4.08, yes. Happy to merge once CI comes back.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

CI is happy. could this please be approved?
Thanks!

@dra27 dra27 merged commit 58cfcb6 into ocaml:trunk Mar 13, 2019

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 Author

commented Mar 13, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

@dra27 is it okay to cherry-pick to 4.08, finally?

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@shindere - I just opened a PR for this, in case there's a delay deciding

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

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.