Skip to content

Dont hardcode path to config.h#465

Merged
jrybar-rh merged 3 commits intopolkit-org:mainfrom
notpeelz:dont-hardcode-path-config-h
Feb 11, 2025
Merged

Dont hardcode path to config.h#465
jrybar-rh merged 3 commits intopolkit-org:mainfrom
notpeelz:dont-hardcode-path-config-h

Conversation

@notpeelz
Copy link
Contributor

Split from #454 as requested.

This is a minor change that requires meson >= 1.4.0, which isn't available in Fedora 39.
F39 becomes EOL on 2024-11-12.

@notpeelz notpeelz mentioned this pull request Jun 26, 2024
@vmihalko
Copy link
Collaborator

vmihalko commented Feb 6, 2025

Could you please resolve the conflicts? Then, we can proceed with the merge.

@notpeelz notpeelz force-pushed the dont-hardcode-path-config-h branch from 2a3d57a to 8ab85eb Compare February 6, 2025 14:10
@notpeelz
Copy link
Contributor Author

notpeelz commented Feb 6, 2025

Done

CI doesn't have meson 1.4.0 yet. Is that a problem?

@notpeelz notpeelz changed the title Dont hardcode path config.h Dont hardcode path to config.h Feb 6, 2025
@jrybar-rh
Copy link
Member

Done

CI doesn't have meson 1.4.0 yet. Is that a problem?

@mrc0mmand: ^

@mrc0mmand
Copy link
Member

mrc0mmand commented Feb 6, 2025

Hmpf, this is quite a big meson version bump and it cuts off some currently still supported distributions, most importantly Ubuntu Noble (24.04, LTS) - released in Apr 2024, supported until Apr 2029, currently has meson 1.3.2, so I'm still not sure if it's worth it.

As I said in #454 (comment) you don't really have to use full path to the config.h file (IIUC; it works that way fine in systemd), which would drop the requirement for the version bump.

@notpeelz
Copy link
Contributor Author

notpeelz commented Feb 6, 2025

Ah... gotta love eternally-outdated distros.

Feel free to close this PR if you don't think it's worth it.

@mrc0mmand
Copy link
Member

We had a discussion with @jrybar-rh and @vmihalko and agreed to go ahead with this change.

@notpeelz if you cherry-pick mrc0mmand@a6beaea on top of this PR it should make the CodeQL check happy and this PR should be ready to go.

notpeelz and others added 3 commits February 11, 2025 09:52
Ubuntu 22.04 (Noble) ships meson 1.3.4 (ATTOW) but we need at least
meson 1.4.0, so install a newer version via pip.
@notpeelz notpeelz force-pushed the dont-hardcode-path-config-h branch from 8ab85eb to 97f80e4 Compare February 11, 2025 14:53
@notpeelz
Copy link
Contributor Author

Oh that's great news. I've rebased and applied your patch.

@jrybar-rh jrybar-rh merged commit 810b5dd into polkit-org:main Feb 11, 2025
15 of 24 checks passed
@notpeelz notpeelz deleted the dont-hardcode-path-config-h branch February 11, 2025 15:12
@jrybar-rh
Copy link
Member

Ah, here seems to be the hiccup 😄
https://github.com/polkit-org/polkit/actions/runs/13275796335/job/37065055620

@mrc0mmand
Copy link
Member

Ah, here seems to be the hiccup 😄 https://github.com/polkit-org/polkit/actions/runs/13275796335/job/37065055620

Ah, whoops, forgot about that one. I'll submit a follow-up.

@mrc0mmand
Copy link
Member

Ah, here seems to be the hiccup 😄 https://github.com/polkit-org/polkit/actions/runs/13275796335/job/37065055620

Ah, whoops, forgot about that one. I'll submit a follow-up.

-> #551

)

compiler_common_flags += ['-include', 'config.h']
compiler_common_flags += ['-include', config_h.full_path()]
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to bump the minimum required version. You do "need" the full path if polkit can ever be built as a subproject -- the compiler is always run from the global_build_root(), that is, the same directory as the build.ninja file, and config.h is generated where current_build_dir() == project_build_root(), which is the same directory as build.ninja except in a subproject.

But regardless, the string config_h.full_path() is provided for API equivalence with other objects. You could have always reliably guaranteed the location quite simply:

config_h_full_path = meson.current_build_dir() / 'config.h'

No version bump needed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's actually pretty neat, thanks a lot for the explanation!

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.

5 participants