search

bazelbuild / rules_scala

Scala rules for Bazel
Apache License 2.0
363 stars 275 forks source link

Stricter unused dependency checking for plus one deps mode #867

Open Jamie5 opened 4 years ago

Jamie5 commented 4 years ago

When using plus one deps mode, many unused deps do get marked, but some deps can be left out because they are already the dep of another dep, and it also makes sense to leave them out because they never appear in the source code of the package being compiled, and the only reason the dep is needed is to make scalac happy. Would it be possible to have a unused deps mode for plus one deps mode where a dep is marked as an unused, unless it is explicitly referenced in the source code? (Not sure if this would end up with a number of false positives - not that familiar with scalac's needs)

This is now in progress, the below summarizes the status

Known potential issues

Things to do

andyscott commented 4 years ago

I haven't had a chance to catch up on this thread, but I'm hoping to take a look tomorrow morning.

ittaiz commented 4 years ago

Assuming no issues from @andyscott and @long-stripe would then try to make some progress. I feel like until we do quash most/all false positives/negatives we would still want the old behavior to be available, so probably there will be a new flag/rule argument to turn on new unused/strict mode.

I think this might be ok but I'm a bit concerned about the complexity. Let's see how it turns out. What we probably need is to extract unused-deps and strict-deps to phases to ease experimentation. Thing is I think doing it before #839 is done might be counterproductive since right now we have a lot of code spread around which relates to label collection and my hunch is it will be a bit of a mess to move it right now. Not sure.

Implementation-wise, I think the first steps would be to merge dependency_analyzer and unused_dependency_checker in a reasonable way, and probably also merge some calling code in a reasonable way. Followed by starting to actually implement.

Sounds good.

Why do we need to test against different scala versions? That sounds like even more complexity... Maybe we should just do #703 (basically move the test that checks the ability to use different versions to 2.11 and move the default to 2.12.10). WDYT?

ittaiz commented 4 years ago

And also sorry for the long delay in replying

Jamie5 commented 4 years ago

I think this might be ok but I'm a bit concerned about the complexity. Let's see how it turns out. What we probably need is to extract unused-deps and strict-deps to phases to ease experimentation. Thing is I think doing it before #839 is done might be counterproductive since right now we have a lot of code spread around which relates to label collection and my hunch is it will be a bit of a mess to move it right now. Not sure.

Ok, will try with flags for now unless #839 finishes before getting there but if it gets horrible will think further.

Why do we need to test against different scala versions? That sounds like even more complexity... Maybe we should just do #703 (basically move the test that checks the ability to use different versions to 2.11 and move the default to 2.12.10). WDYT?

Even if we move to >= 2.12.4 we still need to support 2.12.3 and below until at least whenever their EOLs are, right?

The problem is that we will have different code running for different versions, one way or another. One example is the case of constants, where in the code we have

            node.attachments.get[global.treeChecker.OriginalTreeAttachment].foreach { attach =>
              fullyExploreTree(attach.original)
            }

This cannot work before 2.12.4 because that is the first version in which OriginalTreeAttachment exists. So the code will have to do different things one way or another for 2.12.4+ and 2.12.3-. Then, we want to test both cases (though 2.12.3- would still have the false positive) because we don't want 2.12.3- to break things in strange ways due to the different code paths.

Another example is about a false positive which we discussed above, when method A has a default argument of type B, but the caller does not pass B. And hence B shouldn't be a strict dep. The way to handle this is likely looking at if the compiler calls certain compiler-generated functions to populate the variables (I hadn't seen any better way so far), which afaik have no guarantees between versions. So we want to make sure that the behavior of correctly not requiring a direct dep in this case holds across all versions, even if something changes. Hence the desire to test across all versions that we support.

ittaiz commented 4 years ago

Good questions. I’m not sure and am really concerned about the cost this will have on CI times and maintainability of such tests. Maybe I’m wrong

Jamie5 commented 4 years ago

Okay will try to make sure that parallelism happens with CI when we get to that (IIUC we use travis and already have different bazel versions tested, so hopefully can get that to do different scala versions as well). Not sure that maintainability will necessarily suffer but if so it can hopefully be improved.

ittaiz commented 4 years ago

Travis parallelism is severely limited due to the resources an open source project gets. Basically each bazel version we use doubles our CI times. Not sure we’re talking about the same times here. One option is to only run these tests on Linux (less platform means less time and also Linux is much more available than OSX)

Jamie5 commented 4 years ago

Ok given those constraints we can probably be limited to running only certain scala unit tests against multiple versions.

That would be the changes in UnusedDependencyCheckerTest.scala on e.g. https://github.com/Jamie5/rules_scala/commit/dd699df9d65c4768f09e9135f1f3673d599dd330

and that should catch almost all issues. The tests do run pretty fast at least locally and I think the build-time dependencies are relatively limited. So hopefully the cost is low enough at that point.

Jamie5 commented 4 years ago

@ittaiz created https://github.com/bazelbuild/rules_scala/pull/954 as the first step.

A few things

ittaiz commented 4 years ago

should we use scalatest or junit? One test uses scalatest and one test uses junit. I prefer junit but am fine with whatever the consensus is

Good question- we had a consensus of scalatest but then I broke it because it was more convenient for me to work with specs2. Dear Oscar commented at the time he’d rather keep consistency and he’s probably right. If it’s ok I’d prefer we use scalatest.

We might be able to let people use both strict_deps and unused_deps as a side effect of this all (so I guess transitive dependencies, but also pruning unneeded direct deps). Useful?

What do you mean as a side effect of this? We could have let people use it also before, it didn’t make sense to Stripe people because their main motivation wasn’t build file hygiene but cache hits (which conflicts with many inputs).

Jamie5 commented 4 years ago

Good question- we had a consensus of scalatest but then I broke it because it was more convenient for me to work with specs2. Dear Oscar commented at the time he’d rather keep consistency and he’s probably right. If it’s ok I’d prefer we use scalatest.

Fine with me, updated https://github.com/bazelbuild/rules_scala/pull/954 with the change as it was small and simple.

What do you mean as a side effect of this? We could have let people use it also before, it didn’t make sense to Stripe people because their main motivation wasn’t build file hygiene but cache hits (which conflicts with many inputs).

I think it would have been somewhat trickier due to the differing code paths, which are starting to be merged a bit now. But yes it could always have been done this and some future followup diff should just make it easier to do so. There have been requests such as https://github.com/bazelbuild/rules_scala/issues/235#issuecomment-426773974 to make them work together, though I have it on good authority that the person making that specific request no longer really needs it.

Re the CLA, it is required only for the actual merge and not for anything before that, right?

After this plans are (assuming no objection)

ittaiz commented 4 years ago

SGTM, thanks for working on this!

Jamie5 commented 4 years ago

@ittaiz Wanted to check, what is the current state-of-the-art way to build scala for both 2.11 and 2.12, for libraries which need to build for both versions? I read some things about toolchains while searching around but didn't find anything specific.

Am looking at how to make the unit tests run for many versions, have some ideas but also feel like this might be something that is actually very trivial and I am just really bad at searching for things.

ittaiz commented 4 years ago

There’s no such way- see https://github.com/bazelbuild/rules_scala/issues/940#issuecomment-577464264

ittaiz commented 4 years ago

Also I’m guessing you saw the “switch version e2e” test we have which has a different workspace with 2.12, right?

Jamie5 commented 4 years ago

If you mean test_version.sh then yes.

Ok so IIUC bazel-core will add some features in 2020 which when combined with some rules_scala work will make it so that you can actually build for multiple versions easily. I am down to help with that on the rules_scala side (not sure which specific tasks are involved) though that would be a lower priority than this as I guess we're blocked on core bazel anyways.

Of course that doesn't help us right now but if whatever solution right now needs to live for a shorter time, that will help inform how this current solution should work.

I played around with two solutions

Solution A: Acts similar to test_version, by copying a directory and building it with modified versions. The downside of this is the build times, since there's a bunch of c++ stuff like proto etc which needs building which take a bunch of time. Seen https://github.com/Jamie5/rules_scala/commit/e3653fc69c4b8f22ec6721b0a226c06a233dced8

Solution B: Just modify the root WORKSPACE.scala to use different versions. The downside is this is of course terrible, on the bright side it is definitely faster. Didn't fully time but it seems like something like 30s vs 120s. Seen https://github.com/Jamie5/rules_scala/commit/b4688ec98e641275174cca251f5553510462da4c

I would prefer using solution B but not putting it in the CI build and naming the script something like dangerous_test_thirdparty_version.sh, and explaining it messes with your main workspace file so be careful when running it. And when someone modifies the plugin they really should run the script to make sure everything is fine.

Eventually when we can use the rules_scala support for multiversion, then we can run CI on 2.11.12, 2.11.0, 2.12.0, and 2.12.10 which should catch most issues (or even just 2.11.12 and 2.12.10 if we need to keep the time down). But maybe still have a way to run on everything between 2.11.0-2.12.10 to catch any strange issues that may occur.

Why this preference

ittaiz commented 4 years ago

IINM the bazel-core already allows rules_scala to do all that it needs. Now it's just up to us to do the heavy lifting. @smparkes is working on this (haven't had the chance yet to take a look at their work). I think, but might be wrong, they're not leveraging the core bazel features because they were a bit unclear but according to John Cater all the bits should be in place. @Jamie5 do you want to maybe assist @smparkes with this effort and hold off with this one? I'd understand you being reluctant to do it.

And when someone modifies the plugin they really should run the script to make sure everything is fine

You mean that the contributors.md or something should note this?

JamieRu commented 4 years ago

I think you mentioned

I should say that this is not the main motivation for our move and so I don't want rules_scala to codify a pattern of solving this when Bazel are lined up to solving it in a clear way in the next months (definitely in 2020).

which to me implied there is something in core bazel which was needed but maybe I misunderstood.

Looking at the diff I ended up getting very lost so am probably not the best person to help. (I barely know anything about toolchains). If there are smaller areas of it I might be able to help there though.

If that effort can progress quickly I'm fine just keeping the script not checked in at all but rather run it when I make changes and hope that no one really touches the dependency analyzer in the meantime. But if we did want to check in then I guess somewhere like contributing.md would be reasonable.

Anyways I hope this issue can continue to progress nevertheless, I can also try to help at https://github.com/bazelbuild/rules_scala/issues/940 but that would probably be limited due to skills.

ittaiz commented 4 years ago

I’ll take a look at the B option script again tomorrow

smparkes commented 4 years ago

So there three ways to look at multiscala:

  1. configure the version with a file and change the file and rerun bazel
  2. configure with a command line argument and rerun bazel
  3. configure multiple versions and run all with a single bazel command

I really want to do the last while at the same time making the user configuration, e.g., scala_binary(...) as easy as the first two.

I think @ittaiz is alluding to platform switches. I don't believe they currently support building multiple platforms at the same time, in particular, not with custom switches: the switches don't make it into the filename so you'd end up writing the same file through multiple paths.

That means that multiple bazel runs with different parameters or a run that includes multiple targets based on flags is going to rebuild all targets since they share the same file name. A remote cache should still keep all versions but they'd have to be repulled at switches. I don't think bazel uses a sha-based cache for targets locally (but I could be wrong about that ...)

For us, the third is highly desired because we have to ship our code to people running different versions of the compiler and want our developers to test all that code against all those versions during the normal dev cycle. Since that's been my focus, I haven't looked too much at 1 and 2.

ittaiz commented 4 years ago

Small FYI (since I’d like to centralize the discussion over at #962 )- bazel does have a local sha based solution using the diskcache option

smparkes commented 4 years ago

@ittaiz ack & thanks. I remember seeing this now but hadn't had an immediate use so forgot. It would be relevant for switching configurations. There would be more work copying things around but I don't know how material that would be.

Jamie5 commented 4 years ago

Next step: https://github.com/bazelbuild/rules_scala/pull/967

I did run the script to test each version and it works for all releases from 2.11.9 to 2.11.12 and 2.12.0 to 2.12.10 (2.11.0-2.11.8 are affected by https://github.com/bazelbuild/rules_scala/issues/966 and seems unrelated to this diff).

ittaiz commented 4 years ago

Option B looks reasonable. Do you want to maybe cp the workspace aside and then restore it when the test ends (success or failure)? Right now you do the "cleanup" only if previous tests passed, right?

Jamie5 commented 4 years ago

Yes if a test fails we don't try to restore it. But can make it always restore.

Is this something we do want to check in for use until better multiversion is here? If so I can polish up and create a PR.

ittaiz commented 4 years ago

I think it has value yes. And I’d appreciate the recovery.

Jamie5 commented 4 years ago

Created https://github.com/bazelbuild/rules_scala/pull/971 for the test

Jamie5 commented 4 years ago

Created https://github.com/bazelbuild/rules_scala/pull/975 for simplifying the plugin calling logic (among other logic) but note the additional comment I made.

Jamie5 commented 4 years ago

@ittaiz re your question on https://github.com/bazelbuild/rules_scala/pull/975

Once the AST mode is merged on the plugin side then your plan is to allow people to opt-in to it and activate it for direct, +1 and indirect? Only for some of them?

AST mode would not make sense for direct so I think it should be restricted to plus-one and transitive. (However see footnote at the very bottom) And it would just be a flag that the user sets in toolchain. And once AST mode gets rid of all false positives and no longer has concerns we can remove the user-facing toggle.

Are you planning on changing the +1 collection?

You mean how the +1 deps actually gets collected? No current plans besides what is in https://github.com/bazelbuild/rules_scala/pull/975 ,which should collect whatever information is necessary based on the combination of flags.

====

And now some discussion about user facing flags which I wrote in response to your question before reading it again and realizing you never asked -_- but it is still somewhat relevant

High-level mode would stay around to handle unused-deps even when AST becomes good for +1 and transitive. In theory one could figure out the rules for what needs to be included for the direct case and hence make a better analyzer for that.

I think there would be a few knobs in toolchain

Initially the defaults would be direct, off, off, high-level respectively.

Hopefully we eliminate dependency_tracking_method as a user-facing option once the ast method is stable and all looks good. So that it will automatically be ast for dependency_mode=transitive/plus-one and high-level for unused_deps_checker. However while ast is experimental I think we would just have people be able to tweak it for all

So the three knobs the user adjusts would be dependency_mode, strict_deps, unused_deps. I guess unused_deps will always be off by default, while strict_deps would be on by default for dependency_mode=plus-one/transitive.

And all of this needs to be backwards compatible, but I don't see any problems with that yet.

Footnote: The above discussion of transitive generally treats it as a distinct mode that would be set in toolchain rather than simply a command-line option that can be used to make diagnosing issues easier. If we want to preserve transitive as something that gets passed in commandline, then some things above change. e.g. the analyzer for transitive would need to be high-level.

ittaiz commented 4 years ago

In general sounds great. Two small questions:

High-level mode would stay around to handle unused-deps even when AST becomes good for +1 and transitive.

Why? Why do you think high-level is good enough for direct?

Re the footnote- you lost me. Any chance you can reiterate?

Jamie5 commented 4 years ago

Why? Why do you think high-level is good enough for direct?

I guess to clarify, the requirements are different between direct and plus-one/transitive. So if AST mode works for plus-one, then it won't work for direct. The whole point of +1 deps is not to list a bunch of stuff to make the compiler happy. But for direct we do need to include all that stuff.

High-level isn't necessarily good enough for direct but to handle it someone would need to figure how the rules for direct works. Which would probably be some mode which includes AST's tree walking but expands it a bit further. FWIW for us unused-deps-checker very rarely had false positives for direct though there were definitely some annoyances.

Re the footnote- you lost me. Any chance you can reiterate?

So the discussion was basically assuming you would in the toolchain or somewhere have dependency_mode: "plus_one" or dependency_mode: "transitive", basically a project picks one of plus-one, direct, or transitive and sticks with it.

But (based on my understanding) transitive currently is just something that gets passed into the command line on a build command (as strict_java_deps). And basically you can always run with transitive on or off and the code should compile either way. So in reality there are only two dependency_mode options: direct and plus-one.

So assuming this is actually the behavior we want (that the toolchain only has two dependency_mode options) (at that point we would just keep the existing is_plus_one_on instead of having dependency_mode) then some of the stuff stated above would be incorrect. But I don't think it would be incorrect in a way that makes it not work, it would just need to be adjusted accordingly.

Jamie5 commented 4 years ago

@ittaiz also have for review https://github.com/Jamie5/rules_scala/pull/1 which is built on top of https://github.com/bazelbuild/rules_scala/pull/967 (will move to correct repo once https://github.com/bazelbuild/rules_scala/pull/967 merges, got confused trying to do stacked diffs I guess)

The logic is mostly not focused on https://github.com/bazelbuild/rules_scala/pull/967 and not that intertwined on it (except for being built on top of it) which is why I'm submitting despite the base not being fully reviewed yet.

Jamie5 commented 4 years ago

Ok as it merged https://github.com/bazelbuild/rules_scala/pull/985 is now the proper pull request instead of ^

ittaiz commented 4 years ago

Jamie, Why do you think command line is so different than toolchains? It’s just a different way to configure stuff

Jamie5 commented 4 years ago

It was the impression I got based on a few things such that strict_deps always seemed to override +1 etc and that except for better compile errors it was the same as direct. But maybe I was reading too much into things in which case everything written above works, except maybe that someone wanting transitive will need to pass strict_java_deps as well on the command line to make java rules happy.

ittaiz commented 4 years ago

I've read the above discussion a few times to make sure it makes sense (I hope at least): A few clarifications- The reason the scala strict-deps uses java strict-deps command-line flag is because starlark rules couldn't define command line args 2.5 years ago (we had define but that's a different story). Additionally if I remember correctly toolchains weren't a thing then but I might be wrong.

I think that the best course of action is to expose all 4 knobs you mentioned above in the toolchain. Additionally I think that we should break the user facing API by moving from strict_java_deps on the command line to strict_scala_deps on the command line. Main reason is because we have a weird constraint today where the default java mode is on but for scala it's off. This is weird. This of course should be handled by having an interim period where we support both of them.

WDYT?

Jamie5 commented 4 years ago

To clarify, in the dependency_mode knob on the toolchain, would the options include transitive, or would transitive only be possible by passing strict_scala_deps on the command line (which would set both transitive mode as well as turning on strict deps)?

ittaiz commented 4 years ago

Wdyt we should do? And why?

Jamie5 commented 4 years ago

I think we should just put it on the toolchain as one of the possible modes.

It seems natural and also easier for the user for a decision like this to be on the toolchain with the other settings, so there should be some reason to put it as a strict_scala_deps instead. That reason might be that someone only turns on strict_deps/transitive sometimes to help hunt down missing dependencies, and passing a command line flag is easier than having a second toolchain available which has the dependency_mode flag flipped (though of course, you can do the same with both).

I don't know what all the users of rules_scala do but it seems unlikely that someone would normally build without the flag, and only when running into a cryptic compile error build again with the flag, redoing lots of work. So probably people leave the flag on all the time. If that's the case, I don't think there's much ease to be gotten by having it be a command line flag.

Given that, when the user just has all those options on the toolchain it is easy enough to understand, and they don't think about the interaction of this command line flag with the existing toolchain options.

ittaiz commented 4 years ago

To clarify your above suggestion is to not have any support for a command line flag? Also one usecase that exists is migrations. We (Wix) moved everyone with strict deps warn and then iterated until we had no warnings and moved to error. Most of the iteration was via scripts but IIRC some of it was on people as well which made the flag easier (especially for noobs). Wdyt? Also symmetry with java has a plus

Jamie5 commented 4 years ago

Yes that was the suggestion.

Hmm re migration, what would the difference between using a command line flag and just having a toolchain with dependency_mode=transitive, strict_deps=warn? And then switching to strict_deps=error once all issues were fixed? You mention that the flag is easier for some people to help migration but if it was on the toolchain wouldn't people be getting those warnings anyways?

I suppose symmetry is good, the question is that compelling enough to complicate our model which is already a bit more complex than java-world with +1 deps. It's probably not a clear thing for sure but I would say no.

One more thing - if we want a strict_scala_deps in the future but don't add it now that involves no breaking changes to eventually add it (beside the one of no longer using strict_java_deps which will be breaking in any case). While if we do add a strict_scala_deps but later decide it's kind of annoying to have it, then removing it would be a second breaking change.

I will of course defer to whatever your decision is.

ittaiz commented 4 years ago

Ok. I’m with you. Let’s make sure we document the migration option with a second toolchain in an easy location in the readme

Jamie5 commented 4 years ago

Ok sounds good.

Unrelatedly, have https://github.com/bazelbuild/rules_scala/pull/988 which improves some AST analysis.

Jamie5 commented 4 years ago

Here is the proposed immediate state and final state and how to get there (which was somewhat discussed above but now laid out in more detail since implementation will begin soon)

Immediate state

5 toggles on toolchain

Passing strict_java_deps=warning/error

Changes

  1. Remove strict_java_deps flag overriding things
  2. Remove plus_one_deps_mode
  3. dependency_tracking_method is removed, and ast is used except for dependency_mode=direct.
  4. Change plus_one_deps to have strict_deps=error by default, unless user overrides. At this point strict_deps_mode=default becomes deprecated
  5. Disallow using strict_deps=off for dependency_mode=plus-one. At this point strict_deps_mode=off and strict_deps_mode=default are removed.

1 and 2 can happen whenever a sufficient period of time has passed, and are independent from the remaining changes.

3 probably only happens sometime after ast is deemed stable by whatever definition of stable. 3, 4, 5 should happen sequentially with necessary periods of time in between.

Final state

3 toggles on toolchain

Jamie5 commented 4 years ago

@ittaiz what was the reason for gathering plus_one deps as an aspect (plusone.bzl)?

Reason for asking: There is logic there which checks if we are using plus one (and I think right now it collects the +1 deps even if dependency_mode=transitive). Ideally we could rely on the phase_dependency to provide this information but that means that this aspect would need to be changed to be part of a phase instead.

Jamie5 commented 4 years ago

One more small PR to fix an issue somewhat unrelated to +1 specifically but found during testing: https://github.com/bazelbuild/rules_scala/pull/990

Fun fact: With the outstanding PRs, one more fix which is dependent on an outstanding PR, and a hack to run in ast mode, the number of cases of turning off unused dependency checker/ignoring specific targets in a rather large codebase is 0, and there are no known strict/unused deps issues in the AST mode (the latter is not quite as impressive as after checking the first hundred or so errors, one just then starts grepping and running buildozer commands. But at least nothing is taken out by unused deps checker which prevents compiling)

ittaiz commented 4 years ago

Re immediate:

Default is error if dependency_mode=transitive else off If dependency_mode = transitive, strict_deps=off is disallowed

Not sure I agree with both above bullets. Care to elaborate on the motivation? I elaborate below why I think off value for strict_deps has value in the long term.

Re final:

Will we always want direct to be the default vs e.g. plus-one?

Don't know. Let's defer for now.

strict_deps_mode = error, warn

Why not allow off? Sounds valuable both for people with direct who don't want the overhead and for people with +1/transitive who want the maven flow (which is flawed IMHO but I'm not sure we should disallow it). Also you can do it in java.

still in effect showing all strict deps violations to user, just that for direct there is no such thing as strict deps violations

Does this mean the plugin still runs but you think it won't show anything?

what was the reason for gathering plus_one deps as an aspect (plusone.bzl)?

You want to collect plus one deps from non scala jvm rules like Java/Kotlin/Groovy/etc

Jamie5 commented 4 years ago

Not sure I agree with both above bullets. Care to agree the motivation? I elaborate below why I think off value for strict_deps has value in the long term.

The motivation was that without strict deps checking changing the deps can easily break things for rules which depend on it (i.e. the original motivation of strict deps in the first place) so people shouldn't do it. But if people want the option of the maven flow (which I'm not familiar with) then yeah I guess we should forever allow it.

Does this mean the plugin still runs but you think it won't show anything?

Internally we would turn the option to off so actually it won't run strict deps checking. This is just that as far as the user is concerned, it could well be running the strict deps tool and the user wouldn't notice a difference.

Unrelatedly, https://github.com/bazelbuild/rules_scala/pull/991 fixes one more false positive in the AST checker and I think only one more after that is needed to actually allow ast mode/etc. Of course that is until we are ready to do more of the plan.

Jamie5 commented 4 years ago

And https://github.com/bazelbuild/rules_scala/pull/995 , now with the ability to use the new mode without hacks.

ittaiz commented 4 years ago

Internally we would turn the option to off so actually it won't run strict deps checking. This is just that as far as the user is concerned, it could well be running the strict deps tool and the user wouldn't notice a difference.

If we agree (which I think you agreed) to having strict_deps off mode then your new definition is that when one uses dependency_mode=direct the default strict_deps mode will be off and then the plugin won't run, right?