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 support for the bfd library #9302

Merged
merged 1 commit into from Feb 14, 2020
Merged

Fix support for the bfd library #9302

merged 1 commit into from Feb 14, 2020

Conversation

@shindere
Copy link
Contributor

shindere commented Feb 12, 2020

This PR is a tentative fix for issue #9297.

There was a regression: -ldl was not tried to link with the bfd library.

This PR fixes this regression by re-introducing "-ldl".

@Octachron: if this works, it would be good to include it in 4.10, I think.

@olafhering

This comment has been minimized.

Copy link
Contributor

olafhering commented Feb 12, 2020

It is actually -ldL

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Feb 12, 2020

@olafhering

This comment has been minimized.

Copy link
Contributor

olafhering commented Feb 12, 2020

I was unclear, sorry. Just the description is incorrect,

The patch works, it succeeds with the -lbfd -ldl -liberty -lz -lm case.

@shindere shindere closed this Feb 12, 2020
@shindere shindere deleted the shindere:bfd-ld branch Feb 12, 2020
@shindere shindere restored the shindere:bfd-ld branch Feb 12, 2020
@shindere shindere reopened this Feb 12, 2020
@shindere shindere force-pushed the shindere:bfd-ld branch from 7c19af8 to 936f4e8 Feb 12, 2020
@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Feb 12, 2020

@dra27
dra27 approved these changes Feb 13, 2020
Copy link
Contributor

dra27 left a comment

Just needs a Changes entry.

There was a regression: -ldl was not tried to link with the bfd library.

This commit fixes this regression.
@shindere shindere force-pushed the shindere:bfd-ld branch from 936f4e8 to 4199406 Feb 13, 2020
@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Feb 13, 2020

@Octachron

This comment has been minimized.

Copy link
Member

Octachron commented Feb 13, 2020

Concerning the inclusion in 4.10, since this is a regression fix on a configure option, I am of for inclusion in 4.10 . I will merge and cherry-pick once the Ci is green.

@olafhering

This comment has been minimized.

Copy link
Contributor

olafhering commented Feb 13, 2020

This regression exists also in 4.08 and 4.09. Is this a backport candidate?

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Feb 13, 2020

@Octachron

This comment has been minimized.

Copy link
Member

Octachron commented Feb 13, 2020

There is a 4.09.1 maintenance release planned once I am done with 4.10 . If there is some indication that a 4.08.2 release would help downstream package manager, I am willing to consider it.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Feb 13, 2020

@Octachron Octachron merged commit 9b36ee9 into ocaml:trunk Feb 14, 2020
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 Author

shindere commented Feb 14, 2020

Octachron added a commit that referenced this pull request Feb 14, 2020
Fix support for the bfd library

(cherry picked from commit 9b36ee9)
Octachron added a commit to Octachron/ocaml that referenced this pull request Feb 14, 2020
Fix support for the bfd library

(cherry picked from commit 9b36ee9)
@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Feb 14, 2020

Okay, we now have again a problem on OpenBSD where -ldl is not
recognized. Concretely, it means that configure won't be able to
detect BFD support properly on OpenBSD, which will cause an error in
presence of the --with-bfd configure option.

I am not sure this is a real regression as compared to the pre-autoconf
situation, though, because, as far as I know, nothing confirms that this
actually worked before.

I don't know yet how this should best be fixed.

Will think about it, but any suggestion welcome.

One possibility would be to try -ldl only if the OS is different from
OpenBSD. It wouldn't be too difficult to implement, but not sure that's the
right way to go?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 14, 2020

As someone with no clue about how autoconf works, the intuitive idea would be to first try without -ldl and then try with it, and record in a variable somewhere the flags that actually worked (if any), for use when compiling the actual code that needs the bfd library.

@olafhering

This comment has been minimized.

Copy link
Contributor

olafhering commented Feb 14, 2020

Which AC_CHECK_LIB did succeed before on openBSD? I think just adding the final newly added AC_CHECK_LIB call to cover binutils with --enable-plugns should be enough to avoid regressions. Then the added -ldl can be removed again from the existing AC_CHECK_LIB calls.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Feb 17, 2020

@shindere shindere deleted the shindere:bfd-ld branch Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.