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

Swift: first skeleton extractor #8700

Merged
merged 7 commits into from
Apr 12, 2022
Merged

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Apr 8, 2022

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.

@redsun82 redsun82 requested a review from AlexDenisov April 8, 2022 08:15
@redsun82 redsun82 changed the title Siwft: first skeleton extractor Swift: first skeleton extractor Apr 8, 2022
@redsun82 redsun82 requested a review from criemen April 8, 2022 09:06
AlexDenisov
AlexDenisov previously approved these changes Apr 8, 2022
Copy link
Contributor

@AlexDenisov AlexDenisov left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd get some more eyes on it :)

@redsun82 redsun82 marked this pull request as ready for review April 8, 2022 13:21
@redsun82 redsun82 requested a review from a team as a code owner April 8, 2022 13:21
@redsun82 redsun82 requested a review from MathiasVP April 8, 2022 13:21
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
Copy link
Collaborator

@criemen criemen left a comment

Choose a reason for hiding this comment

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

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.

cpp/BUILD.bazel Outdated Show resolved Hide resolved
swift/README.md Show resolved Hide resolved
swift/BUILD.bazel Show resolved Hide resolved
srcs = ["//swift:extractor-pack"],
args = [
"--destdir",
current_source_dir() + "/extractor-pack",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain what you mean with "what was done in ruby"? I have not looked at their extractor build at all.

Copy link
Contributor Author

@redsun82 redsun82 Apr 12, 2022

Choose a reason for hiding this comment

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

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 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(which by the way allows cleaning up the pack directory, which might otherwise pose problems with leftover files)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :-)

swift/extractor/BUILD.bazel Show resolved Hide resolved
misc/bazel/workspace.bzl Show resolved Hide resolved
misc/bazel/BUILD.toolchain.tpl Outdated Show resolved Hide resolved
misc/bazel/test.bzl Outdated Show resolved Hide resolved
criemen
criemen previously approved these changes Apr 12, 2022
Copy link
Collaborator

@criemen criemen left a comment

Choose a reason for hiding this comment

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

LGTM, with the expectation of a follow-up PR that does the workspace renaming dance.

swift/extractor/BUILD.bazel Show resolved Hide resolved
@redsun82 redsun82 merged commit 03ebf8b into github:main Apr 12, 2022
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