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

compilers: Detect Vala compiler for the requested machine #10735

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oleavr
Copy link
Contributor

@oleavr oleavr commented Aug 24, 2022

Instead of unconditionally detecting it for the build machine.
The rationale for making this use the build machine was wrong.
The valac transpiler needs search paths for the host machine
(specifically, the --vapidir default paths).

This reverts part of af846a1.

@oleavr oleavr requested a review from dcbaker as a code owner August 24, 2022 15:43
@eli-schwartz
Copy link
Member

This seems wrong and the argument in the other PR for why you want to use the build machine is pretty sensible IMO.

I'm not sure why you want to revert it, it basically sounds like "but I want to do it wrong"? It basically lacks explanation.

...

What's wrong with specifying it in the native file? The PR should explicitly rationalize why you find that to be unsatisfactory, rather than leaving reviewers to try to guess.

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (adf09b8) 70.23% compared to head (9f8b501) 50.42%.

❗ Current head 9f8b501 differs from pull request most recent head 3e2c3f8. Consider uploading reports for the commit 3e2c3f8 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #10735       +/-   ##
===========================================
- Coverage   70.23%   50.42%   -19.82%     
===========================================
  Files         219      203       -16     
  Lines       48534    44152     -4382     
  Branches    11516     9786     -1730     
===========================================
- Hits        34090    22264    -11826     
- Misses      12009    19887     +7878     
+ Partials     2435     2001      -434     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oleavr
Copy link
Contributor Author

oleavr commented Aug 24, 2022

The PR should explicitly rationalize why you find that to be unsatisfactory, rather than leaving reviewers to try to guess.

Oops, indeed. Apologies.

What's wrong with specifying it in the native file?

It couples the choice to what's chosen for the build machine, when e.g. pkgconfig is decoupled from it. Meson currently assumes that if it finds a dependency through pkgconfig, there will be a corresponding .vapi on the Vala compiler's vapi search path.

I think it would make sense to fall back to using the build machine's selection though, but unconditionally coupling things seems unnecessary.

@eli-schwartz
Copy link
Member

eli-schwartz commented Aug 24, 2022

Ah okay, so the vapidir is host machine specific.

So the issue (and the commit message) is not/should not be:

The cross-file may want to specify a particular compiler.

But rather:

The rationale for making this use the build machine was wrong. The valac transpiler needs search paths for the host machine (specifically, the --vapidir default paths).

@eli-schwartz
Copy link
Member

I wonder if it's possible to somehow detect this. Probably setting the cross file to ['/usr/bin/valac', '--vapidir', '/path/to/sysroot/usr/share/vala/vapi/'] or something would allow using a native version anyway. It might be possible to be clever about this.

@oleavr oleavr force-pushed the fix/vala-compiler-detection branch from a9abd9c to cb15ef1 Compare August 24, 2022 18:04
@oleavr
Copy link
Contributor Author

oleavr commented Aug 24, 2022

Ah okay, so the vapidir is host machine specific.

So the issue (and the commit message) is not/should not be:

The cross-file may want to specify a particular compiler.

But rather:

The rationale for making this use the build machine was wrong. The valac transpiler needs search paths for the host machine (specifically, the --vapidir default paths).

Thanks! Just updated the issue description and commit message.

Instead of unconditionally detecting it for the build machine.
The rationale for making this use the build machine was wrong.
The Vala compiler needs search paths for the host machine
(specifically, the `--vapidir` default paths).

This reverts part of af846a1.
@oleavr oleavr force-pushed the fix/vala-compiler-detection branch from cb15ef1 to 3e2c3f8 Compare February 12, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants