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 --with-pic in configure #8632

Merged
merged 2 commits into from May 21, 2019

Conversation

Projects
None yet
3 participants
@dra27
Copy link
Contributor

commented Apr 20, 2019

@dbuenzli noted in #7678 (comment) that specifying --with-pic causes -fPIC to appear twice when using ocamlc -c. While fixing that PR, I noticed two problems with the logic for --with-pic:

  • It should be updating $internal_cflags, not $common_cflags (which is what Daniel noticed)
  • aspp is not used anywhere - I think it should be updating ASPP, although I also wonder if it should in fact update default_aspp (which would require ASPP to be defined later. @shindere - if the user provides ASPP, do we expect that to have absolutely everything which is required? If so, then I should amend this to set default_aspp instead. As it stands, if the user says ./configure ASPP=foo --with-pic then Makefile.config will have ASPP=foo -fPIC.
@shindere

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@damiendoligez damiendoligez added this to the 4.08 milestone May 6, 2019

@damiendoligez

This comment has been minimized.

Copy link
Member

commented May 7, 2019

I tend to agree: if the user decides to override ASPP, don't add anything to it.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented May 20, 2019

ping @dra27, this is (almost) the last thing holding up 4.08.0.

dra27 and others added some commits Apr 20, 2019

Fix --with-pic in configure
--with-pic should be adding -fPIC (or equivalent) to $internal_cflags,
not $common_cflags. Additionally, unused variable aspp was being
updated, instead of ASPP.

@damiendoligez damiendoligez force-pushed the dra27:fix-with-pic branch from cf5edea to cb83f68 May 20, 2019

@damiendoligez

This comment has been minimized.

Copy link
Member

commented May 20, 2019

@dra27 I have pushed to your branch to change to default_aspp as discussed and update Changes (and rebase). Do you agree with the changes?

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Sorry for my slowness with this - thanks for the rebase @damiendoligez, I agree with the changes, and everything looks fine.

@dra27 dra27 merged commit c37a6e5 into ocaml:trunk May 21, 2019

2 checks passed

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

@dra27 dra27 deleted the dra27:fix-with-pic branch May 21, 2019

dra27 added a commit that referenced this pull request May 21, 2019

Fix --with-pic in configure (#8632)
--with-pic should be adding -fPIC (or equivalent) to $internal_cflags,
not $common_cflags. Additionally, unused variable aspp was being
updated, instead of ASPP.

(cherry picked from commit c37a6e5)

dra27 added a commit that referenced this pull request May 21, 2019

Fix --with-pic in configure (#8632)
--with-pic should be adding -fPIC (or equivalent) to $internal_cflags,
not $common_cflags. Additionally, unused variable aspp was being
updated, instead of ASPP.

(cherry picked from commit c37a6e5)
@dra27

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

4.08: 0c33a66
4.09: dc3c056

@damiendoligez

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Thanks a lot!

smuenzel added a commit to smuenzel/ocaml that referenced this pull request Jun 26, 2019

Fix --with-pic in configure (ocaml#8632)
--with-pic should be adding -fPIC (or equivalent) to $internal_cflags,
not $common_cflags. Additionally, unused variable aspp was being
updated, instead of ASPP.
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.