Swift: first skeleton extractor#8700
Conversation
AlexDenisov
left a comment
There was a problem hiding this comment.
LGTM, but I'd get some more eyes on it :)
7cddfba to
df3bb74
Compare
This adds a first dummy extractor for swift. Running `bazel run //swift:install` will create an `extractor_pack` directory in `swift`. From that moment providing `--search-path=swift` will pick up the extractor.
When importing the workspace from semmle-code, we do not need nor want to instantiate `@util`, so that must be in a separate bazel package.
* fixed 5.0.0 as bazel version * made dependencies better loadable * moved `//swift/install` to `//swift:create-extractor-pack` (following the clearer ruby naming) * renamed `extractor_pack` to `extractor-pack` for consistency with Ruby
df3bb74 to
f2f9961
Compare
criemen
left a comment
There was a problem hiding this comment.
Some comments about the bazel stuff.
I don't know enough about the rest of the extractor pack setup to review for example the manifest and qlpack files.
| srcs = ["//swift:extractor-pack"], | ||
| args = [ | ||
| "--destdir", | ||
| current_source_dir() + "/extractor-pack", |
There was a problem hiding this comment.
I wonder if we should move this from the hermeticity-breaking current_source_dir rule to the documentation instead.
That would allow us to get rid of the extra repository, and of somebody accidentally using current_source_dir anywhere else.
What do you think?
There was a problem hiding this comment.
yeah, I was also a bit wary of this, then decided to go with what was done in ruby. We could skip the --destdir and point .clodeqlmanifest to the corresponding place in the target's runfiles (as we do not need to worry about window here)...
There was a problem hiding this comment.
Can you explain what you mean with "what was done in ruby"? I have not looked at their extractor build at all.
There was a problem hiding this comment.
they have a create-extractor-pack.sh script that when run without any arguments will instantiate the extractor pack in ruby/extractor-pack, which in turn is referenced by ruby/.codeqlmanifest.json.
Any way, I found a more elegant solution by wrapping the install script setting DESTDIR using BUILD_WORKSPACE_DIRECTORY, so the scary current_source_dir() can go away 🙂
There was a problem hiding this comment.
(which by the way allows cleaning up the pack directory, which might otherwise pose problems with leftover files)
There was a problem hiding this comment.
Yeah that's the reason we do the whole delete+unzip in sembuild all the time - having a good solution that's a bit faster would be a good model for something to adopt there, too :-)
criemen
left a comment
There was a problem hiding this comment.
LGTM, with the expectation of a follow-up PR that does the workspace renaming dance.
This adds a first dummy extractor for swift.
Running
bazel run //swift:installwill create anextractor_packdirectory in
swift. From that moment providing--search-path=swiftwill pick up the extractor.