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.
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
Kotlin: build extractor with bazel #16117
Kotlin: build extractor with bazel #16117
Changes from 67 commits
0da4130
341816c
c242466
47ff1c1
5313288
55ff710
5d6baea
3a0a219
cbdb1eb
44f3c02
7aefd22
38a7bc0
fd77f1a
e963b84
a970c2d
60febcd
b71ffc6
4a4bd16
11729aa
4b205ff
9c73a9a
662fd5c
e7c680e
3bdab70
aca8d04
c9565b3
5bdd724
a15681a
35a2ed8
1a7f25a
5df1abc
24ef424
fe35902
02257ee
f0fc811
5415665
3b75d63
c18b556
7f495b1
59fdbdb
3d14654
fc62ed5
b8c063e
4822de3
a834101
c64d02d
bc89742
9c3a615
2d16192
9114131
b8010f2
65df2bb
0ad8ed3
24c7ad5
5c2d9fe
a78124b
92a5f3d
b36cabb
bdc8a7f
9d1901c
b07fa70
e53ef4a
1b5675e
6a83bf9
5bb2cba
bd631c5
8c705ad
735b341
aee3c0d
27ab487
306f0f1
a741170
c5f6c65
3678e51
7952f0e
f685843
aaa29d8
072e2ed
5b143ce
c014cd8
4aa0a8e
a23327c
1e622e1
8e1d77b
a841a2b
d7ecaae
450f651
b834173
d4e0a56
52a015f
239b6d8
514e24c
e546560
75709bf
eab940c
821bd1f
d1a2c0f
99f70a6
fcd326e
6a9cb90
2fe0718
4c91bdc
e7cec01
e693c27
c6039b3
be5c82c
8205f86
a48d71b
10584b3
e4653a8
b7e16ca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Would
fetchexclude = *
be equivalent? That would more natural to me.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.
yeah, agreed, will do!
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.
ah, I remembered why I had gone for
fetchinclude
rather thanfetchexclude
: the latter has precedence over the former, so if one wants to override these settings in order to fetch some specific files it becomes non trivial.Say we start having more of such lazy LFS files (I'm planning to use this for Swift as well), and you want to fetch the kotlin files. With
fetchinclude=/nothing
in.lfsconfig
, it suffices for you togit config lfs.fetchinclude=/java/kotlin-extractor/**
, which will override the/nothing
and give you what you want, while other files will stay excluded.With
fetchexclude=*
in.lfsconfig
, you would need to (say) do something likegit config lfs.fetchexclude=swift/**
, or exclude all directories save for/java
just to be forward compatible with the exclusion rules...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.
Ah, OK, that makes sense. Could you add a comment explaining that please?
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.
It looks like this can't be done from
<root>
, but can be done from<root>/ql
or<root>/ql/java/kotlin-extractor
. Is that expected?Also, I think that this should mention that you needs
tools/bazel
to be the bazel on your path.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 can add a note about building from the internal repo (
bazel build @codeql//java/kotlin-extractor:etc
is what works from there)Actually, any installed bazel or bazelisk will do (bar http fetching problems 😅). On
codeql
, if it's bazelisk (which is now the recommendedbazel
executable to have and is whatbazel
is on github actions) it will pick up the correctbazel
version. If it's a fixedbazel
version, it will exit any way with a helpful error on a version mismatch.Furthermore, on
semmle-code
, any installedbazel
will actually auto-picktools/bazel
.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.
The helpful error will have the user install bazel, though, and they will have to update their installation when we bump our bazel version. Wouldn't it be better to just tell them to use the tools/bazel they already have?
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.
does this work repeatedly? Presumably, the env variable needs to change for bazel to run the configuration again, no?
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.
Indeed, this requires
CODEQL_KOTLIN_SINGLE_VERSION
to appear empty in the environment for bazel to pick up a change. For example setting that in a persistent env won't help at all.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.
Can we add a note about this then, and/or drop this workaround?
Or maybe shell out to a source of randomness to set the variable content :D
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.
Nit: Can we call this
target
instead?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.
Nit doesn't seem to have been addressed, despite a thumbs-up emoji