-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add static as keyword to find_library #3747
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
Conversation
Would it make sense to have different keywords like |
Yeah I guess there are four combinations
I don't think "require anything" can be implemented on windows because there are no ways to determine if the found library is static or shared. EDIT: I've fixed this in the next commit. We can use An additional thing. I noted that meson itself produces lib<name>.a for static libraries on windows and <name>.dll for shared libraries. Which is an additional reason to have the right priority on the prefixes. |
65e00eb
to
6e4ed2f
Compare
82aa05a
to
be47d51
Compare
mesonbuild/compilers/c.py
Outdated
if shlibname in stdout: | ||
return True | ||
return False | ||
raise AssertionError('BUG: unkonwn libtype: {!r}'.format(libtype)) |
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.
unkonwn -> unknown
mesonbuild/compilers/c.py
Outdated
if libtype == 'static': | ||
if shlibname in stdout: | ||
return False | ||
return True |
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.
I think you could just write return (shlibname not in stdout)
here, and likewise the negation of that below...
mesonbuild/compilers/c.py
Outdated
# FIXME: shared-static and static-shared return the same for now | ||
if libtype in ('shared-static', 'static-shared'): | ||
return True | ||
proc = subprocess.Popen(['lib', '/list', filename], stdout=subprocess.PIPE) |
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.
I'm not sure how safe the assumption that lib
is in the path is. Perhaps the potential exception raised if it can't be found should be handled.
I guess there is no possibility of a cross-tool being required.
This conflicted directly with #3691, so it needs a rebase. I will review it after that. |
04bec59
to
23ab3bd
Compare
mesonbuild/compilers/c.py
Outdated
stdout = stdout.decode('utf-8') | ||
shlibname = filename.with_suffix('.dll').name | ||
if libtype == 'static': | ||
return not shlibname in stdout |
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.
[Flake8]
[E713] test for membership should be 'not in'
45934b5
to
c081c20
Compare
Thanks for the comments. I've updated and rebased. Ready for review @nirbheek |
Test failure seems to be relevant. |
5891c3a
to
86c4ad3
Compare
@nirbheek Fixed test failure |
ping! |
I guess this will need another rebase |
Before rebasing I would like to know if the PR is desired or not. |
86c4ad3
to
599d886
Compare
rebased |
599d886
to
a1ee6b1
Compare
57b0fb9
to
b56579f
Compare
OK, i've rebased and changed the kwarg to static. There are changes in this PR that are not needed to actually merge the static-kwarg feature. Do you want me to move any such changes to another PR? |
That's always a good idea for minimizing delays in getting a PR merged. Small PRs are easy to review and merge :) |
True forces static, False forces dynamic.
0a6db0b
to
2d8d656
Compare
Have not looked at the implementation but featurewise this is ok. |
Tests passes on my machine now. Waiting for review. |
One failure seems related. Is it on purpose that it links statically to
|
I can't reproduce the error locally. I installed msys2 and mingw-x86_64 and that test passes on my machine. |
I spoke to soon, it fails, but I get another error:
|
yay, I fixed the bug on CI. The issue I had on my machine was unrelated. |
0.50? |
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.
In general I think this is a nice change, there is a little bit of user facing language changes that @jpakkane should look at. We should probably also extend dependency()
to understand the "either but prefer X" syntax at some point as well.
We're past feature freeze for 0.50, and only a couple of days away from release so I think this will need to wait for 0.51. Please also write up a documentation snippet and the make the doc changes to the manual and reference manual about this new keyword. Though let's not do that until we get done bikeshedding the name.
I blamed it, the |
It is (still) an implementation detail, so I can change it if there is consensus on a better name. There is quite a high probability that something like this will sneak out into the public API and changing it now might nudge the potentially public API into a better direction. |
Added some docs |
I'll send a separate PR for the rename. Test failures look unrelated BTW. |
This patch allows the build file author to enforce static or dynamic linkage. I've found this to be necessary when the two versions are not equivalent.
AFAIK there are not really any naming standards when it comes to libraries on Windows. But from my experience it is more common to prefix the static version with 'lib' and not have any prefixes for the dynamic library. This patch therefore changes the order of the prefixes so that dynamic libraries are found first which seem to be the case already on other platforms.