https://pantsbuild.org/ logo
Join Slack
Powered by
At the moment, it looks like dependencies of JVM s...
# development
a
At the moment, it looks like dependencies of JVM source files are assumed to be able to be turned into a 
ClasspathEntry
, which is not true for 
file
targets. As I go through and break this assumption, I’m wondering if I should be silently ignoring all non-classpath dependencies when assembling the classpath, or if I should have some way of affirmatively opting into this behaviour. (@witty-crayon-22786 @fast-nail-55400)
w
so… iirc there is already a way. let me look.
…oh. no. i was thinking of https://github.com/pantsbuild/pants/pull/13911:
To fix this, we add support to 
ClasspathEntryRequest
for “root only” requests, which allow for requesting a classpath for a target when it is a root, but failing when the target type is used as an inner node (since we don’t want `deploy_jar`s to be used as dependencies of JVM compile targets anytime soon).
a
Yup 🙂
w
As I go through and break this assumption, I’m wondering if I should be silently ignoring all non-classpath dependencies when assembling the classpath, or if I should have some way of affirmatively opting into this behaviour.
good question. so far, confirming that we should ignore is mostly a precautionary measure to make sure that we don’t miss cases. but it will rule out some cases that we can’t imagine, so loosening it over time, or removing the requirement entirely might be reasonable.
a
OK, I think my approach, then, will be to fire warnings for anything that doesn’t have a 
FileSourceField
w
hm.
f
(side note: You may to check codegen to see if convertible to 
FileSourceField
since 
relocated_files
is implemented that way.)
w
@fast-nail-55400: that should go hand in hand with applying the static logic from codegen to all of 
ClasspathEntryRequest
👍 1
@ancient-vegetable-10556: i wouldn’t special case 
files
, per-se… an approach based on installing a “noop” 
ClasspathEntryRequest
type would be good. then, if there was going to be a warning, it would be something like “there are no ClasspathEntryRequest types for `$target`” (which wouldn’t trigger for 
files
, because we’d have a noop installed)…?
a
Yup, I think I’m fine with using an empty 
ClasspathEntry
in places
seems a bit weird, but should work
w
yea… it morphs the meaning of the type a little bit. it’s more like “FieldSetTypeThatTheJVMBackendIsAwareOf”
f
AbstactJvmFieldSetProviderBean
w
too true =x
a
I’m going to go off and cry in a corner now Tom
f
it’ll get better in a few months … it’ll be Spring then
a
see above
@witty-crayon-22786 This minor yak shave emerged while figuring out how do the above dependency work. I think it makes things clearer in javac/scalac support code
👍 1
What do I have do to make the generated 
file
targets from 
relocated_files
appear?
(other than check for a 
RelocatedFiles
target, which seems brittle)
h
so we don't actually generate 
file
targets. we generate a 
Snapshot
of files that "look like" the sources of 
file
targets (this is actively being investigated for JVM to support compiled languages)
w
@hundreds-father-404: a random thought i had on the subject was that FieldSet applicability should maybe be codegen aware
a
I can’t see how we’re materialising a digest for 
relocated_files
, I copied the 
golang
code almost directly and my relevant snapshot was empty
(so the support may be broken in 
golang
tests too?)
w
@ancient-vegetable-10556: could be. rather than consuming a 
files
target, code that wants to consume 
FilesSources
should be making a `SourcesRequest`… the target that gets passed to the 
SourcesRequest
needs to be one that has a 
SourcesField
subclass which is convertible-to 
FilesSources
.
h
For Golang, support for 
files
is limited to 
go/goals/test.py
. Implemented at lines 284-294, key detail being 
enable_codegen=True
a
I HAVE COPIED THAT CODE 🙂
it is not capturing files output from 
relocated_files
, per the tests in there
h
and my relevant snapshot was empty
What, in a test? You have to make sure 
core/target_types.py
rules() is activated for codegen rule of 
relocated_files
to happen
a
let me check that
h
btw it would be excellent to explicitly test 
relocated_files
for Go 
test
support. That's an oversight we aren't doing so
a
understood
explicitly adding in the 
rules()
from 
core/target_types.py
does not appear to do anything either
adding the same test in go fails in the same way
w
i think that punting on 
relocated_files
for the purposes of your current change should be fine, because it’s very overlapped with Eric’s codegen work
a
OK. It would be good to verify whether existing golang support works properly here too
w
golang will also need updates for compiled-language codegen… it probably won’t completely fall out of that work, but opening a ticket about 
relocated_files
for go and jvm and marking it blocked by https://github.com/pantsbuild/pants/issues/14258 would be reasonable.
a
@witty-crayon-22786 I’ve done that, and have marked the relevant test as `skip`ed to make it easier for whoever discovers the ticket
👍 1
Open in Slack
PreviousNext
Powered by