https://pantsbuild.org/ logo
Join Slack
Powered by
Does this sound like it’d be a reasonable approach...
# general
f
Does this sound like it’d be a reasonable approach to the problem that java_sources on scala_libraries do not cause scala_libraries to invalidate? https://github.com/pantsbuild/pants/issues/7200#issuecomment-460435452
a
i am being dumb but is there a reason we can’t just allow both java and scala files in 
sources
?
f
we do
but some repo layouts don’t expect them in the same subtree
so to have java/scala together, you need to do something awkward
and the current awkward solution has sharp edges
a
i don’t understand what the repo layouts are that don’t expect them in the same subtree or why that needs special handling
f
eg
src/java/…. and src/scala/….
a
i'm going to look into the zinc compile task to understand why that needs special handling on the pants side
f
it needs special casing because current pants doesn’t deal well with single compilation units with separate source roots
^^ would do away with most of the special casing
because we’d set up the dependency relations in a more reasonable direction and make it so that depending on the carry-along sources will fail
a
so i don't know what source roots are or why the "source root" is anything but the target base dir and never have understood this
f
so, imagine you have a layout like
Copy code
src/java/org/foo/a
src/scala/org/foo/a
if you want src/scala/org/foo/a to also compile the java sources at the equivalent path in src/java, you need it to have a ref to the other path
because the target base dirs are different
a
does the compiler require the sources to be in directories matching package names? because if so this makes sense and i hate java
f
yes
but it’s not a strict requirement
it just causes warnings
more modern layouts for java sometimes look like
src/java/com.foo.bar/
where the prefixes are common to eg artifacts or something
a
i'm not too into using the 
files
target since it is real code and not a resource and that feels pretty confusing
f
would you prefer to have a new type for it, like #65 proposed?
a
does the compiler allow dropping all the files into a single dir and calling it a day?
f
yes
but sometimes it’ll warn or error
a
(looking at #65 more, just got in from a fire alarm)
f
also if you have classes with the same name at different pkg namespaces, that won’t work
a
and does the compiler (i'm testing this out with zinc on the command line momentarily) require file names to be anything particular too? i feel like the answer is yes if not i was just going to shade them
if this is a bad idea i won't derail more
f
yes-ish
I think you need to have the file name match at least one public class and maybe only one public class
I’d have to check again
a
i can figure this out and see if flattening everything is viable in any way
i don't understand why we want to avoid introducing a new target type and instead use 
files
f
another option in general would be to have 
src/something-for-both-java-and-scala/....
as a source root
but this feature allows us to support existing dir structures
I’m not sure whether we need to avoid a new target type, but the type would be like 
files
in that it’s a set of files that is at rest unless another target says its going to use it for something
I think conceptually it’s like deferred sources
a
that makes sense
w
i expect that v2 will change this entirely, so diving in too deep right now isn't worthwhile
a
and yeah i don't want to impose any specific dir structure on people ideally and i despise how most IDE authors think forcing people to maintain a specific dir structure is ok
will v2 solve how to send files to the compiler and respect source roots? i don't understand the relevance here, this just seems like an interface issue
f
i expect that v2 will change this entirely, so diving in too deep right now isn’t worthwhile
^^ is another reason for using 
files
and making the smallest semantic change possible that eliminates the issues. It also could simplify some of the things we’re trying to convert to v2
w
i guess what i'm trying to say is: this is a very low priority issue right now
f
yeah. It’s got an id’d workaround and a proposal for a fix
a
yeah i just didn't know that source roots were so annoying lol
zinc cares about file names but not at all where the files are or at least our zinc wrapper does when i invoked it just now
it creates the appropriate output directories for you, e.g. 
com/twitter/...
and drops the classfiles in there but it does that regardless of where the input file is located, depending just on the declared 
package com.twitter;
so it might not be the case that we need to compile sources relative to source roots when using zinc
and that seems like it solves the problem and allows code removal
f
we need it for a number of reasons, not just pleasing the compiler, eg resources
have you looked at the docs around source roots?
a
i didn't think about resources
f
also, there’s a correctness thing in that since the compiler doesn’t care about package vs file location you can have the wrong package in a file as compared to its location
source roots are also a big issue for dynamic languages
a
zinc creates the tree of nested directories for compiled class files from the 
package
declaration, not from the file path
f
yes
but for users, having the source files organized by package can be helpful
a
i extremely don't want to impose any requirements on the way users structure their projects, which is why the way most IDEs require users to structure their projects like that appalls me
oh wait and a target can only have one source root atm?
f
hm. I have mixed feelings. I think that like a linter, having conventions around directory structure enforced by automation is a net positive
but different people are going to want different conventions and different languages have different conventions
a
i think conventions are fantastic! i think requiring every user of a language to use your specific convention because your IDE is lazy is awful
f
so Pants needs to be able to support a number of different ones
targets generally only associate with one source root
I think moving towards an idea of groups of sources with a common root being distinct from targets makes sense though
🔥 1
a
making a 
sources_by_source_roots()
method which returns an 
OrderedDict
and deprecating 
sources_relative_to_source_root()
sounds like a start
f
hm. I’m not sure whether that’s strictly necessary, or if most tools would be cool with code from multiple source roots together as a blob
but otoh, having that and flattening it is easy
and that structure would allow for some interesting things
a
i think it would resolve the current issue
and pave the way for deprecating 
java_sources
to just be 
sources
f
hm. maybe
but that doesn’t fix the problem because you still need a ref to another place, unless you are ok with mixing file types in a dir
to be clear, I would probably prefer just saying the sources files for a particular compile unit should just live together
but we support other structures too
maybe sources could allow for pointing at targets--I’m not sure I like that
a
i have not and will never in my entire life ever have the opinion of "let's require users to conform to this arbitrary convention to reduce implementation difficulty" it is literally why i exist i am trying to see if there is a way to allow the use of 
sources
or 
java_sources
without having the current invalidation issue
f
oh, ok
a
i don't understand what you mean by "ref" here
f
so, there’s a particular code layout convention that we’re trying to support with the 
java_sources
feature
pants doesn’t allow source references outside the targets declaration path
so, another target must own the sources that are outside the `scala_library`’s dir if it is to use them
but that target should only be referred to by the consuming target for compile purposes
a
thinking of 
java_sources
in the current incarnation, deferred sources seems like a very appropriate analogy or at least i think that whenever i see a target kwarg that accepts another target spec
i did this for unpacking stuff from python wheels
f
yeah
I think it needs to be separate from 
sources
because it’s points to targets and 
sources
is globs
a
yeah i did actually read #65 but forgot that 
java_sources
was referring to targets
it feels like making a task to populate 
deferred_sources
might work here? looking at how we currently handle 
java_sources
this is what you said all the way at the top i am aware
if we allow something like 
unpacked_jars()
to wrap 
java_library()
targets (
interdependent_java()
?) for use in `scala_library()`s in either the 
java_sources
kwarg or a new one (and then make a tiny task that populates 
deferred_sources
), that also gives us the possible extra benefit of being able to only depend on specific files within the 
java_library()
target with 
include_patterns
. that may not be useful (so we could just make it always include 
**/*
), but it would be safe from the current bug since only those files would be visible to the scala target.
plus having more infrastructure use deferred sources is a good thing imo i think it is a very flexible hammer
f
Hmm. I feel like the general pattern here should be more like having a general way to say a target uses sources from another target.
a
that is exactly what i am describing so maybe i am not being clear
remote_sources()
and 
unpacked_jars()
is exactly this, the only difference is that we wouldn't need to unpack any jars since `java_library()`s are just sources
f
I'm not sure about having source selectors that point at a subset of another target s sources
a
then we can not expose any such selector
f
The issue with java_sources, is that the targets they point to are partial
a
ok right i forgot that
files
might be fine then to make that crystal clear, i would still prefer even an alias for 
files
like 
java_files
but i think the proposal sounds appropriate because it is probably important to denote that these aren't real targets quote unquote
f
Yeah. There's some further semantics to work out, but I think the graph shape bits are right
👌 1
a
cool i think this is great
6 Views
Open in Slack
PreviousNext
Powered by