-
Notifications
You must be signed in to change notification settings - Fork 140
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
Introduce a Swift wrapper of the C++ runfiles library #1470
Open
cerisier
wants to merge
11
commits into
bazelbuild:master
Choose a base branch
from
cerisier:cerisier/cpp_runfiles_library_wrapper
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Introduce a Swift wrapper of the C++ runfiles library #1470
cerisier
wants to merge
11
commits into
bazelbuild:master
from
cerisier:cerisier/cpp_runfiles_library_wrapper
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cerisier
requested review from
aaronsky,
BalestraPatrick,
brentleyjones,
keith,
luispadron,
mattrobmattrob and
thii
as code owners
December 29, 2024 14:32
fmeum
reviewed
Dec 29, 2024
fmeum
reviewed
Dec 29, 2024
cerisier
force-pushed
the
cerisier/cpp_runfiles_library_wrapper
branch
from
January 2, 2025 12:28
776d6e1
to
e2af4f0
Compare
cerisier
changed the title
Introduce a Swift C++ runfiles library wrapper
Introduce a Swift wrapper of the C++ runfiles library
Jan 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Second shot at implementing #890
This PR introduces a Swift runfiles lookup library.
After discussion, it was decided to give a go to wrapping the C++ runfiles library instead of writing one in pure Swift.
Implementation
This implementation is done in 2 parts:
C++ wrapper as C API
I took the decision to expose the C++ runfiles library to Swift via a C API for the following reasons:
The original C++ runfiles library was not compatible with Swift C++ interop out of the box:
std::unique_ptr
in the public API (https://www.swift.org/documentation/cxx-interop/#unique-reference-types)It would fail with an error like this:
note: record 'Runfiles' is not automatically available: does not have a copy constructor or destructor; does this type have reference semantics?
Using C++ interop currently forces users to provide
-cxx-interoperability-mode=default
down the line, otherwise the header from the generted module map is treated as C and not C++.For those reasons, I chose the C API approach which makes everything invisible to users of the library.
Swift Wrapper
The Swift wrapper is just a wrapper around around the C API with 3 exceptions:
CommandLine.arguments[0]
automatically instead of requiring users to passargv0
like it is the case in the C++ library.rlocation
returnsnil
instead of an empty string if repository mappings fail.Source Repository
The runfiles library needs to know the canonical name of the repository that is trying to resolve a particular runfile path.
This is required to map apparent repository names used in the runfile path, to its canonical repo name on the filesystem.
Every ruleset has its own way of making this transparent to the users of the runfiles library (Except C++ and Java), most of them rely on runtime accessors or language specific features that do not exist in Swift.
Here, we rely on the built-in #filePath magic identifier which has the property to be expanded at callsite when used as a default argument, the same way
assert
andfatalError
work.I couldn't find the specification for this behavior except the below mention in an accepted Swift proposal:
https://github.com/swiftlang/swift-evolution/blob/80c3616bb23e7bf751f886aaf178acb3df09abe5/proposals/0274-magic-file.md#provide-separate-modulename-and-filename-magic-identifiers:
From that #filePath, we deduce the canonical repository name the same way
rules_go
does (thanks @fmeum).See https://github.com/bazel-contrib/rules_go/blob/6505cf2e4f0a768497b123a74363f47b711e1d02/go/runfiles/global.go#L53-L80
Additional notes
I didn't include tests at this stage on purpose before we settle on the API. Also, because this is a wrapper reusing a battle tested C++ library, I would like to discuss which tests make sense to write :)
Because we always compute the source canonical repository name, I didn't expose variants of the C++ API that do not require it.
In the original pure Swift implementation, I had
rlocation
return aURL
instead of aString
, let me know if I should use that instead.Finally, all rulesets providing a runfiles library provide a
env
andargs
attributes to*_binary
that are subject to Make Variable substitution (for$(rlocationpath //target) most notably)
, let me know this is something we want and if I should submit a PR for that.TODO
Links