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 linker nonsense partly #9731

Closed
wants to merge 2 commits into from

Conversation

Neumann-A
Copy link
Contributor

closes #9730
closes #9727

In my opinion the whole linker detection is a mess... there is no way for a user to opt out of the "meson" behavior......

@codecov
Copy link

codecov bot commented Dec 15, 2021

Codecov Report

Merging #9731 (02ab5e6) into master (a2934ca) will decrease coverage by 3.07%.
The diff coverage is n/a.

❗ Current head 02ab5e6 differs from pull request most recent head 606c23f. Consider uploading reports for the commit 606c23f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9731      +/-   ##
==========================================
- Coverage   67.08%   64.01%   -3.08%     
==========================================
  Files         404      202     -202     
  Lines       85711    42814   -42897     
  Branches    18907     9357    -9550     
==========================================
- Hits        57501    27406   -30095     
+ Misses      23676    13189   -10487     
+ Partials     4534     2219    -2315     
Impacted Files Coverage Δ
mesonbuild/compilers/detect.py
mesonbuild/linkers/detect.py
mesonbuild/backend/vs2015backend.py
mesonbuild/depfile.py
mesonbuild/compilers/compilers.py
mesonbuild/interpreter/primitives/__init__.py
mesonbuild/compilers/mixins/elbrus.py
mesonbuild/mesonlib/__init__.py
mesonbuild/modules/__init__.py
mesonbuild/ast/introspection.py
... and 193 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2934ca...606c23f. Read the comment docs.

@@ -292,9 +292,9 @@ def detect_static_linker(env: 'Environment', compiler: Compiler) -> StaticLinker
linkers = default_linkers
popen_exceptions = {}
for linker in linkers:
Copy link
Member

@dcbaker dcbaker Dec 15, 2021

Choose a reason for hiding this comment

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

Why not just add

linker = os.path.basename(linker)

Here and call it good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

linker is a list?
Also this assumes again that the linker is on the path which it is not.....

override = comp_class.use_linker_args(value[0])
check_args += override

if extra_args is not None:
check_args.extend(extra_args)

p, o, _ = Popen_safe(compiler + check_args)
p, o, _ = Popen_safe(compiler + check_args) # This assume whatever 'compiler' is, is either on PATH or it is an absolute path to a valid executable!
Copy link
Contributor

Choose a reason for hiding this comment

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

assumes

@@ -72,13 +72,14 @@ def guess_win_linker(env: 'Environment', compiler: T.List[str], comp_class: T.Ty
override = [] # type: T.List[str]
value = env.lookup_binary_entry(for_machine, comp_class.language + '_ld')
if value is not None:
compiler = value # If meson is explicitly told a value for the linker so use it!
Copy link
Contributor

Choose a reason for hiding this comment

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

If meson is explicitly told a value for the linker, use it!

Copy link
Member

Choose a reason for hiding this comment

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

This change makes me nervous, this should only get hit in the case of gcc-esque clang, which expects to get the linker value as -fuse-ld=..., so I think this is going to break things.

if value is not None and invoked_directly:
compiler = value
# We've already hanedled the non-direct case above

Copy link
Member

Choose a reason for hiding this comment

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

Same here, now we're looking for linkers that are invoked directly, not through the compiler driver. The more I liok at this, the more I feel ilke this change is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I explicitly tell meson to use the linker directly it should shut up and do as I say!
meson really tries too hard to be clever .... (personally I think the whole stuff should be rewritten. I saw many fragments of code duplication, especially for defaults in complier/detect.py and linker/detect.py )

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I'm concerned, meson has to be clever - if you "just use the linker directly" how is it going to know what flags to use for linking? Path conversion, library search locations, whether the target is static or shared etc... There is no concept of a dumb linker because it isn't sensible, I think.

One of my biggest pain points with meson is also one of its biggest strengths - if your compiler is not known, you are forced to implement it properly so all future consumers of that compiler benefit from correct detection.

@Neumann-A Neumann-A closed this Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

meson cannot handle full paths in ar (windows) Error configuring: meson being silly in linker detection.
4 participants