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

binutils: Only embed zlib if listed as a build dependency #1732

Merged
merged 2 commits into from May 17, 2019

Conversation

geimer
Copy link
Contributor

@geimer geimer commented May 17, 2019

Addresses #1350 in conjunction with easybuilders/easybuild-easyconfigs#8340

This does not address the issue that libz.a is included in libbfd.a when specified as a build dependency. At the moment, I don't believe that this is fixable at all without rewriting parts of the Makefile(s).

easybuild/easyblocks/b/binutils.py Outdated Show resolved Hide resolved
easybuild/easyblocks/b/binutils.py Outdated Show resolved Hide resolved
@boegel boegel added this to the next release (3.9.1) milestone May 17, 2019
@boegel boegel added the change label May 17, 2019
@boegel
Copy link
Member

boegel commented May 17, 2019

@geimer Since we're currently only including zlib as a build dep, this doesn't actually change much for existing easyconfigs (except when people have their own binutils easyconfigs where zlib is a runtime dep already), right?

I'm aware that we're changing that in easybuilders/easybuild-easyconfigs#8340, just trying to make sure I have a good view on this w.r.t. backwards compatibility, since the idea is to include this with EasyBuild v3.9.1...

How much of a problem is it that libz.a is still embedded in libbfd.a?
In other words, if we merge this together with easybuilders/easybuild-easyconfigs#8340, is #1350 actually fixed, or not?

@geimer
Copy link
Contributor Author

geimer commented May 17, 2019

Since we're currently only including zlib as a build dep, this doesn't actually change much for existing easyconfigs (except when people have their own binutils easyconfigs where zlib is a runtime dep already), right?

It should be a drop-in replacement with existing easyconfigs, correct.

How much of a problem is it that libz.a is still embedded in libbfd.a?

With the modified easyconfigs for newer binutils, this should only happen for binutils built with dummy, which is only used for bootstrapping GCCcore and binutils built with that GCCcore (and stuff explicitly using the dummy-built binutils, but these should be rare, no?). Thus it's unlikely to be a huge problem. If you want that also fixed, I currently only see two options: (a) require zlib as an OS dependency for the dummy-built binutils and not embed anything (thus dancing around the conflicts and module switch problems) or (b) mess around in the Makefiles even more. While (b) may be doable, it requires much more investigation, i.e., postponing the fix to some release in the future.

In other words, if we merge this together with easybuilders/easybuild-easyconfigs#8340, is #1350 actually fixed, or not?

With both PRs merged, #1350 should be fixed for everything built on top of GCCcore using the binutils built with that GCCcore, as the latter no longer embeds zlib.

Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, will test this extensively both with existing easyconfigs and easybuilders/easybuild-easyconfigs#8340 before merging...

@boegel
Copy link
Member

boegel commented May 17, 2019

Tested with a range of existing binutils easyconfigs old & new (which still have zlib as a build-only dep), no problems found.
Working test reports for easybuilders/easybuild-easyconfigs#8340 are under way...

@boegel
Copy link
Member

boegel commented May 17, 2019

No surprises during testing, so going in, thanks @geimer!

@boegel boegel merged commit 4231f24 into easybuilders:develop May 17, 2019
@geimer geimer deleted the binutils_without_embedded_zlib branch May 18, 2019 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants