https://pantsbuild.org/ logo
Powered by
f

fancy-queen-20734

02/04/2019, 10:27 PM
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

aloof-angle-91616

02/04/2019, 10:40 PM
i am being dumb but is there a reason we can’t just allow both java and scala files in 
sources
?
f

fancy-queen-20734

02/04/2019, 10:41 PM
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

aloof-angle-91616

02/04/2019, 10:43 PM
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

fancy-queen-20734

02/04/2019, 10:43 PM
eg
src/java/…. and src/scala/….
a

aloof-angle-91616

02/04/2019, 10:45 PM
i'm going to look into the zinc compile task to understand why that needs special handling on the pants side
f

fancy-queen-20734

02/04/2019, 10:46 PM
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

aloof-angle-91616

02/04/2019, 10:47 PM
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

fancy-queen-20734

02/04/2019, 10:48 PM
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

aloof-angle-91616

02/04/2019, 10:49 PM
does the compiler require the sources to be in directories matching package names? because if so this makes sense and i hate java
f

fancy-queen-20734

02/04/2019, 10:49 PM
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

aloof-angle-91616

02/04/2019, 10:51 PM
i'm not too into using the 
files
target since it is real code and not a resource and that feels pretty confusing
f

fancy-queen-20734

02/04/2019, 10:51 PM
would you prefer to have a new type for it, like #65 proposed?
a

aloof-angle-91616

02/04/2019, 10:52 PM
does the compiler allow dropping all the files into a single dir and calling it a day?
f

fancy-queen-20734

02/04/2019, 10:52 PM
yes
but sometimes it’ll warn or error
a

aloof-angle-91616

02/04/2019, 10:52 PM
(looking at #65 more, just got in from a fire alarm)
f

fancy-queen-20734

02/04/2019, 10:52 PM
also if you have classes with the same name at different pkg namespaces, that won’t work
a

aloof-angle-91616

02/04/2019, 10:56 PM
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

fancy-queen-20734

02/04/2019, 10:57 PM
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

aloof-angle-91616

02/04/2019, 10:58 PM
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

fancy-queen-20734

02/04/2019, 11:00 PM
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

aloof-angle-91616

02/04/2019, 11:03 PM
that makes sense
w

witty-crayon-22786

02/04/2019, 11:04 PM
i expect that v2 will change this entirely, so diving in too deep right now isn't worthwhile
a

aloof-angle-91616

02/04/2019, 11:04 PM
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

fancy-queen-20734

02/04/2019, 11:05 PM
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

witty-crayon-22786

02/04/2019, 11:06 PM
i guess what i'm trying to say is: this is a very low priority issue right now
f

fancy-queen-20734

02/04/2019, 11:07 PM
yeah. It’s got an id’d workaround and a proposal for a fix
a

aloof-angle-91616

02/04/2019, 11:16 PM
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

fancy-queen-20734

02/04/2019, 11:23 PM
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

aloof-angle-91616

02/04/2019, 11:24 PM
i didn't think about resources
f

fancy-queen-20734

02/04/2019, 11:25 PM
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

aloof-angle-91616

02/04/2019, 11:26 PM
zinc creates the tree of nested directories for compiled class files from the 
package
declaration, not from the file path
f

fancy-queen-20734

02/04/2019, 11:26 PM
yes
but for users, having the source files organized by package can be helpful
a

aloof-angle-91616

02/04/2019, 11:28 PM
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

fancy-queen-20734

02/04/2019, 11:29 PM
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

aloof-angle-91616

02/04/2019, 11:30 PM
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

fancy-queen-20734

02/04/2019, 11:30 PM
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

aloof-angle-91616

02/04/2019, 11:33 PM
making a 
sources_by_source_roots()
method which returns an 
OrderedDict
and deprecating 
sources_relative_to_source_root()
sounds like a start
f

fancy-queen-20734

02/04/2019, 11:34 PM
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

aloof-angle-91616

02/04/2019, 11:35 PM
i think it would resolve the current issue
and pave the way for deprecating 
java_sources
to just be 
sources
f

fancy-queen-20734

02/04/2019, 11:36 PM
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

aloof-angle-91616

02/04/2019, 11:38 PM
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

fancy-queen-20734

02/04/2019, 11:39 PM
oh, ok
a

aloof-angle-91616

02/04/2019, 11:39 PM
i don't understand what you mean by "ref" here
f

fancy-queen-20734

02/04/2019, 11:39 PM
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

aloof-angle-91616

02/04/2019, 11:42 PM
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

fancy-queen-20734

02/04/2019, 11:43 PM
yeah
I think it needs to be separate from 
sources
because it’s points to targets and 
sources
is globs
a

aloof-angle-91616

02/04/2019, 11:45 PM
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

fancy-queen-20734

02/05/2019, 7:05 PM
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

aloof-angle-91616

02/05/2019, 7:05 PM
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

fancy-queen-20734

02/05/2019, 7:06 PM
I'm not sure about having source selectors that point at a subset of another target s sources
a

aloof-angle-91616

02/05/2019, 7:06 PM
then we can not expose any such selector
f

fancy-queen-20734

02/05/2019, 7:07 PM
The issue with java_sources, is that the targets they point to are partial
a

aloof-angle-91616

02/05/2019, 7:07 PM
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

fancy-queen-20734

02/05/2019, 7:09 PM
Yeah. There's some further semantics to work out, but I think the graph shape bits are right
👌 1
a

aloof-angle-91616

02/05/2019, 7:09 PM
cool i think this is great
6 Views
Powered by