-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Introduce static_to_shared_library function #6092
Conversation
@tgamblin I would really like to see |
86a8838
to
1f85abe
Compare
Small update: I moved it into the package class, which has the benefit of giving us access to the spec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, several questions/thoughts:
- How well does this work across different compilers? Are the arguments portable?
- Does this require that the static lib already be PIC? Or does it work in general?
- This seems like something that should only exist in the build environment (though if there are other uses maybe we should rethink that). Maybe the right place for it is in
build_environment.py
, and it could be a function added to the module scope in, e.g.,set_module_variables_for_package()
. You have thePackage
andspack_cc
there, so you could construct a converter function specialized for the build. - Needs a test.
1f85abe
to
5fae802
Compare
I have refactored the code a bit, put it into
From what I could find, at least GCC, Intel and Clang should support
The static library has to be compiled with PIC.
Done. I moved the compiler selection out of the function to be able to test it more easily.
I added a rough first test for it that can be extended later. Please check whether this is what you had in mind. |
Open questions:
|
5fae802
to
69f8050
Compare
No idea, to be honest. Someone else will have to take care of this, as I do not have access to a Windows build environment.
This is just meant as an easy way to convert existing static libraries to shared ones, in case the package in question does not build them (see lua). Instead of patching the package, we can just use this function.
Sorry, I skipped them for the initial version. I just pushed an updated version with support for specifying a version.
I think it should be pretty easy to extend the function to support additional compilers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me -- I think we can punt on windows for the moment, but there are still a few minor requests.
os.environ['SPACK_TEST_COMMAND'] = 'dump-args' | ||
|
||
expected = { | ||
'linux': '/bin/mycc -Wl,-rpath,/spack-test-prefix/lib -Wl,-rpath,/spack-test-prefix/lib64 -shared -Wl,-soname,{2} -Wl,--whole-archive {0} -Wl,--no-whole-archive -o {1}', # noqa: E501 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split across multiple lines (no NOQA)
|
||
class BuildEnvironmentTest(unittest.TestCase): | ||
|
||
def setUp(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use a pytest
fixture for this? We still have some old unittest
style tests but we're trying to switch to pytest
. You can probably just make a build_environment
fixture that does all the same stuff as this, and yields a named tuple with cc
, ld
, etc. It should probably also unset the environment variables after yielding so as not to interfere with other tests.
if 'SPACK_DEPENDENCIES' in os.environ: | ||
del os.environ['SPACK_DEPENDENCIES'] | ||
|
||
def test_static_to_shared_library(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is fine but can be moved to module scope if it's a pytest test. It should take the build_environment
fixture as a parameter.
lib/spack/spack/build_environment.py
Outdated
soname += '.{0}'.format(version) | ||
|
||
compiler_args = [ | ||
'-shared', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a TODO in here somewhere that says these arguments should really be added to the various compiler classes and generalized for different compilers?
lib/spack/spack/build_environment.py
Outdated
Converts a static library to a shared library. | ||
|
||
Parameters: | ||
static_lib (str): Path to the static library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a note here or in the description that this library must be built with PIC.
20c7fb2
to
2786557
Compare
I have pushed a new version. It includes some updates regarding the version handling (if we set a version |
2786557
to
bdbc3e0
Compare
Thanks! LGTM! |
I wonder why the CI tests are failing, they work fine locally. Is there something special about the CI environment? |
# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved. | ||
# LLNL-CODE-647188 | ||
# | ||
# For details, see https://github.com/llnl/spack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update url to spack/spack
!
The static_to_shared_library function takes an existing static library and produces a shared library based on it.
bdbc3e0
to
88bd0a7
Compare
OK, I think I have found the problem ( |
This is work in progress based on the discussion in #6008.
@junghans: Sorry for the timing. I was just working on this when you merged #6008. :-)
The
static_to_shared_library
function takes an existing static library and produces a shared library based on it.For now, I just put it into the lua package. I would like to get some feedback whether the function's interface etc. are what you had in mind. Additionally, I would need some hints on how to integrate this better into Spack. (I have already experimented with putting it into
build_environment.py
but I currently need access tospack_cc
anddso_suffix
.)