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.

Sign up for GitHub

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

Jump to bottom

Hermetic pkg config #979

Merged
merged 12 commits into from
Nov 9, 2022
Merged

Hermetic pkg config #979

merged 12 commits into from
Nov 9, 2022

Conversation

jheaff1
Copy link
Collaborator

@jheaff1 jheaff1 commented Nov 3, 2022

This PR adds a a pkg-config toolchain built from source.

On Windows, pkg-config has a couple of shared library dependencies, therefore the runnable_binary macro is utilised. As such this PR is built on top of PR #971.

There are many changes in this PR, I have tried to produce atomic commits to make each commit easier to review. If the git commit history should be tidied further, please let me know.

FYI - Once this is merged I will then be submitting a PR to add meson support, which depends on this PR

@jheaff1 jheaff1 force-pushed the hermetic_pkg_config_tidy branch 5 times, most recently from 1a51be9 to 461821b Compare November 3, 2022 15:15
@jsharpe
Copy link
Member

jsharpe commented Nov 3, 2022

@jheaff1 thanks for tackling this - I've started this a few times and never quite got round to finishing it so glad to see it being done!
+1 on getting meson support in too.

@jheaff1
Copy link
Collaborator Author

jheaff1 commented Nov 4, 2022

@jheaff1 thanks for tackling this - I've started this a few times and never quite got round to finishing it so glad to see it being done!
+1 on getting meson support in too.

No worries. Thanks for reviewing the runnable_binary PR. The main difficulty with this one was getting it working on Windows. Typical 😂

@jheaff1 jheaff1 force-pushed the hermetic_pkg_config_tidy branch 5 times, most recently from d111d89 to 8acf7cb Compare November 4, 2022 13:31
@jheaff1
Copy link
Collaborator Author

jheaff1 commented Nov 4, 2022

@jsharpe CI is now passing apart from the Bazel 4.0.0 tests. The issue seems to be that providing $(execpath ) to ctx.resolve_command fails. This must be a bug that was fixed in Bazel 5.

Any thoughts on how best to move forward? Could we upversion the minimum version examples to bazel 5? I presume Bazel 4.x is not yet end-of-life as Bazel 4.2.3 was released last month.

I had to replace the usage of ctx.expand_locations with ctx.resolve_command to facilitate the usage of tools built as a sh_binary or py_binary. This currently only applies to the pkg-config toolchain (built as a sh_binary) but my approach to building Meson is to do so as a py_binary.

@jheaff1 jheaff1 marked this pull request as ready for review November 7, 2022 09:15
@jsharpe
Copy link
Member

jsharpe commented Nov 9, 2022

@jsharpe CI is now passing apart from the Bazel 4.0.0 tests. The issue seems to be that providing $(execpath ) to ctx.resolve_command fails. This must be a bug that was fixed in Bazel 5.

Any thoughts on how best to move forward? Could we upversion the minimum version examples to bazel 5? I presume Bazel 4.x is not yet end-of-life as Bazel 4.2.3 was released last month.

I had to replace the usage of ctx.expand_locations with ctx.resolve_command to facilitate the usage of tools built as a sh_binary or py_binary. This currently only applies to the pkg-config toolchain (built as a sh_binary) but my approach to building Meson is to do so as a py_binary.

Have you tried bumping the minimum version to 4.2.3 to see if the fix has been backported at all?

@jheaff1
Copy link
Collaborator Author

jheaff1 commented Nov 9, 2022

@jsharpe CI is now passing apart from the Bazel 4.0.0 tests. The issue seems to be that providing $(execpath ) to ctx.resolve_command fails. This must be a bug that was fixed in Bazel 5.
Any thoughts on how best to move forward? Could we upversion the minimum version examples to bazel 5? I presume Bazel 4.x is not yet end-of-life as Bazel 4.2.3 was released last month.
I had to replace the usage of ctx.expand_locations with ctx.resolve_command to facilitate the usage of tools built as a sh_binary or py_binary. This currently only applies to the pkg-config toolchain (built as a sh_binary) but my approach to building Meson is to do so as a py_binary.

Have you tried bumping the minimum version to 4.2.3 to see if the fix has been backported at all?

Yeah I tried that but alas the issue still occurs

@jheaff1
Copy link
Collaborator Author

jheaff1 commented Nov 9, 2022

Bazel 6 is due out this month I believe, perhaps once it is released we change the minimum supported version of this repo to Bazel 5.x?

@jheaff1
Modify runnable_binary to facilitate generation of runnable that matches
343952e 
the underlying binary name
@jheaff1
Fix runnable binary macro
26f93ee 
Previously transtive shared library dependencies were not included in
the runnable binary runfiles
@jheaff1
Increase hermeticity by using libraries built by bazel in
35f7172 
runnable_binary rather than those already on the library path
@jheaff1
Increase hermeticity by using tools built/fetched by bazel in
e374139 
rather than those already on the PATH
@jheaff1
Facilitate usage of built tools that are py_binary or sh_binary targets
7540382
@jheaff1
Fix builds on Windows that require setting the INCLUDE and/or LIB
7933fc1 
variables
@jheaff1
Add runfiles of tools to the build action, facilitating tools that
98e458c 
require runfiles
@jheaff1
Add hermetic built pkgconfig toolchain
bfefff6 
The registration of the preinstalled toolchains have been moved to the
preinstalled_toolchains macro, so that the hermetic pkgconfig toolchain
is the default.

As the hermetic toolchains are registered first, there is no need to
specify register_preinstalled_toolchains=False in order for the build to
be as hermetic as possible.
@jheaff1
Add pkgconfig toolchain to "make" builds
06b8ac7 
This is required now that the default pkgconfig toolchain is
hermetically built from source
@jheaff1
Change rules_foreign_cc_dependencies so that by default the built
387e0be 
pkgconfig toolchain is not registered, as a few extra bazel flags are
required on Windows.
@jheaff1
Resolve linting error of undocumented args
bac1869
@jheaff1 jheaff1 force-pushed the hermetic_pkg_config_tidy branch 2 times, most recently from 74c2fb8 to 9a5b17a Compare November 9, 2022 10:55
@jheaff1
Copy link
Collaborator Author

jheaff1 commented Nov 9, 2022

@jsharpe I managed to get it building on bazel 4.0.0 by adding a special flag, please see my latest commit for details

jsharpe
jsharpe approved these changes Nov 9, 2022
View reviewed changes
Copy link
Member

@jsharpe jsharpe left a comment

Choose a reason for hiding this comment

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

Thanks; this is awesome work @jheaff1.

@jsharpe jsharpe merged commit 2c6262f into bazel-contrib:main Nov 9, 2022
@jheaff1
Copy link
Collaborator Author

jheaff1 commented Nov 9, 2022

Thanks; this is awesome work @jheaff1.

You're most welcome. Next up, Meson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsharpe jsharpe jsharpe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jheaff1 @jsharpe