-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Bazel: Use new packaging rules #16623
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@criemen: I think we can put a TODO
to remove defs.bzl
and its codeql_platform
after this, right? It's still used in the internal repo, but won't for a long time I think.
go/BUILD.bazel
Outdated
env = {"REPO_NAME": repo_name()}, | ||
main = "create_extractor_pack.py", | ||
deps = ["_extractor_pack"], | ||
install_dest = "build/codeql-extractor-pack", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbg are you particularly attached to this installation destination? If we can go with the default (which would be go/extractor-pack
) it would be consistent with other languages already using that (ruby
, ql
and swift
), and we could go for a generic */extractor-pack/qlpack.yml
include in codeql-workspace.yml
and a generic /*/extractor-pack
exclude in .gitignore
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware of any reason why this has to be codeql-extractor-pack
. I suspect it may just be an inconsistency from when the Go extractor lived in a separate repository. It should be fine to change this to be the same as for other languages. CC @smowton @owen-mc in case they have any insights I don't have.
It turns out we still need to supply this option, so `codeql` goes looking for the extractor paths specified in the `codeql-workspace.yml` file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this! I left a comment for your consideration, but I'm also happy with merging this 🙂 You answered that already. You're fast 😉
This PR updates the external repo to use the new packaging rules introduced in #16432. This removes a rather large dependency on the internal build.
Depends on #16622, and will have to be rebased on top of
main
once that PR has landed.No approval from the individual language teams is required, as this is a purely packaging-related PR that doesn't touch any implementation.