Rearranging java packages & build rules?

[Dave Borowitz]

In a big series I have going, I've been splitting up the //gerrit-server:server target and moving stuff around between packages & top-level directories. There are still plenty of incremental improvements that can be made, but I'm wondering if we should also think about how to completely rearrange the directory layout.

The current layout of having gerrit-* directories with separate java source trees dates back to the bad old days of Maven, when this specific layout was necessary to decompose the build into separate pom.xml files. This is no longer a requiement.

Here are some issues with the current layout:

* A single gerrit-*/BUILD with many rules globbing separate subdirectories makes BUILD files messier, since we have to extract various srcs lists as top-level constants and exclude them from other rules.

* Different gerrit-* directories occasionally contain sources in the same java package, despite the java_library rules being completely separate.

* In general, if you're adding a new class, it's hard to tell which top-level directory it belongs in, unless you are already quite familiar with the package hierarchy.

That said, there are also some advantages:

* Top-level directories give a hint as to what kind of external dependencies are brought in: gerrit-httpd depends on Jetty, gerrit-cache-h2 depends on H2, gerrit-gwt-* depends on GWT.

* Each artifact published on Maven Central corresponds to a single top-level directory: gerrit-war, gerrit-plugin-api, gerrit-extension-api.

A simpler layout would be something like we use internally at Google with Blaze:

* java/com/google/gerrit/... contains Java sources and resources

* javatests/com/google/gerrit/... contains Java tests

* BUILD files exist as close to the *.java files referenced in their rules as possible (our acceptance tests already do this)

* other top-level directories (lib, polygerrit, tools, etc.) stay the same.

Thoughts?

Once we agree on an end goal, we can talk in more detail about how we get from here to there :)

[Han-Wen Nienhuys]

The google layout seems simpler to me, so I am in favor of it.

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Google Germany GmbH, Erika-Mann-Strasse 33, 80363 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle

 

[Saša Živkov]

On Thu, Aug 10, 2017 at 6:32 PM, 'Dave Borowitz' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

In a big series I have going, I've been splitting up the //gerrit-server:server target and moving stuff around between packages & top-level directories. There are still plenty of incremental improvements that can be made, but I'm wondering if we should also think about how to completely rearrange the directory layout.

The current layout of having gerrit-* directories with separate java source trees dates back to the bad old days of Maven, when this specific layout was necessary to decompose the build into separate pom.xml files. This is no longer a requiement.

Here are some issues with the current layout:

* A single gerrit-*/BUILD with many rules globbing separate subdirectories makes BUILD files messier, since we have to extract various srcs lists as top-level constants and exclude them from other rules.

Looks like this is more related to a single BUILD file  and less to the gerrit-* structure?

* Different gerrit-* directories occasionally contain sources in the same java package, despite the java_library rules being completely separate.

* In general, if you're adding a new class, it's hard to tell which top-level directory it belongs in, unless you are already quite familiar with the package hierarchy.

That said, there are also some advantages:

* Top-level directories give a hint as to what kind of external dependencies are brought in: gerrit-httpd depends on Jetty, gerrit-cache-h2 depends on H2, gerrit-gwt-* depends on GWT.

* Each artifact published on Maven Central corresponds to a single top-level directory: gerrit-war, gerrit-plugin-api, gerrit-extension-api.

 

Correct. And opposite is not true: each top-level directory is not published on maven-central.

A simpler layout would be something like we use internally at Google with Blaze:

* java/com/google/gerrit/... contains Java sources and resources

* javatests/com/google/gerrit/... contains Java tests

* BUILD files exist as close to the *.java files referenced in their rules as possible (our acceptance tests already do this)

* other top-level directories (lib, polygerrit, tools, etc.) stay the same.

Thoughts?

Where would the gerrit-war/BUILD, gerrit-plugin-api/BUILD and gerrit-extension-api/BUILD go in the proposed Google layout?

 

Once we agree on an end goal, we can talk in more detail about how we get from here to there :)

In general I am always in favor of removing any complexity which doesn't justify itself any more.

Therefore: +1 

[Martin Fick]

On Thursday, August 10, 2017 12:32:38 PM 'Dave Borowitz' via

Repo and Gerrit Discussion wrote:
> Here are some issues with the current layout:
> * A single gerrit-*/BUILD with many rules globbing
> separate subdirectories makes BUILD files messier, since
> we have to extract various srcs lists as top-level
> constants and exclude them from other rules. * Different
> gerrit-* directories occasionally contain sources in the
> same java package, despite the java_library rules being
> completely separate. * In general, if you're adding a new
> class, it's hard to tell which top-level directory it
> belongs in, unless you are already quite familiar with
> the package hierarchy.

Yes, the current layout is difficult, part of the balme lies
with long java package names, not just the build system.
But not just the package names, also the src/main/java
prefixes that sit in front of the already long package names.
And part of the problem is not just the length of the names,
but the number of divisions, each extra directory results in
one more click or one more tab completion for developers.

+1 for seeking improvements!

> That said, there are also some advantages:
> * Top-level directories give a hint as to what kind of
> external dependencies are brought in: gerrit-httpd
> depends on Jetty, gerrit-cache-h2 depends on H2,
> gerrit-gwt-* depends on GWT.

This is indeed a strong advantage, how would a new layout
deal with this?

-Martin

--
The Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, hosted by The Linux Foundation

[Dave Borowitz]

On Thu, Aug 10, 2017 at 3:04 PM, Martin Fick <mf...@codeaurora.org> wrote:

On Thursday, August 10, 2017 12:32:38 PM 'Dave Borowitz' via
Repo and Gerrit Discussion wrote:
> Here are some issues with the current layout:
> * A single gerrit-*/BUILD with many rules globbing
> separate subdirectories makes BUILD files messier, since
> we have to extract various srcs lists as top-level
> constants and exclude them from other rules. * Different
> gerrit-* directories occasionally contain sources in the
> same java package, despite the java_library rules being
> completely separate. * In general, if you're adding a new
> class, it's hard to tell which top-level directory it
> belongs in, unless you are already quite familiar with
> the package hierarchy.

Yes, the current layout is difficult, part of the balme lies
with long java package names, not just the build system.
But not just the package names, also the src/main/java
prefixes that sit in front of the already long package names.
And part of the problem is not just the length of the names,
but the number of divisions, each extra directory results in
one more click or one more tab completion for developers.

True, thanks Java. I wonder how much bazel cares about this part of the layout; maybe we could flatten it further so gerrit/foo contains java files declaring package com.google.gerrit.foo.

 

+1 for seeking improvements!

> That said, there are also some advantages:
> * Top-level directories give a hint as to what kind of
> external dependencies are brought in: gerrit-httpd
> depends on Jetty, gerrit-cache-h2 depends on H2,
> gerrit-gwt-* depends on GWT.

This is indeed a strong advantage, how would a new layout
deal with this?

Partly, this info is just encoded in the package names, rather than the top-level directory names. E.g. only com.google.gerrit.httpd.* gets a Jetty dependency; it doesn't need to be in gerrit-httpd to suggest that.

Also, note that having a simpler overall layout makes further refactoring to make package names more sane and consistent easier :)

Another answer is we can use bazel's visibility restrictions to help enforce any convention we decide on. If we really strongly feel that Jetty shouldn't be visible outside of com.google.gerrit.httpd, then we just add this to the //lib:jetty rule:

  visibility = ["//java/com/google/gerrit/httpd:__pkg__"]

So even if we lose some ability to guess the dependency structure at a glance, we can still prevent packages from picking up dependencies they shouldn't.

[Martin Fick]

On Thursday, August 10, 2017 03:11:04 PM Dave Borowitz
wrote:

> On Thu, Aug 10, 2017 at 3:04 PM, Martin Fick
<mf...@codeaurora.org> wrote:
> >
> > Yes, the current layout is difficult, part of the balme
> > lies with long java package names, not just the build
> > system. But not just the package names, also the
> > src/main/java prefixes that sit in front of the already
> > long package names. And part of the problem is not just
> > the length of the names, but the number of divisions,
> > each extra directory results in one more click or one
> > more tab completion for developers.
> True, thanks Java. I wonder how much bazel cares about
> this part of the layout; maybe we could flatten it
> further so gerrit/foo contains java files declaring
> package com.google.gerrit.foo.

I suspect that javac will complain about that. But that
would be awesome. Perhaps a way to achieve this would be to
lay it out as you suggest and then provide a soft link from
the long name javac expects?

> >
> > > That said, there are also some advantages:
> > > * Top-level directories give a hint as to what kind of
> > > external dependencies are brought in: gerrit-httpd
> > > depends on Jetty, gerrit-cache-h2 depends on H2,
> > > gerrit-gwt-* depends on GWT.
> >
> > This is indeed a strong advantage, how would a new
> > layout
> > deal with this?
>
> Partly, this info is just encoded in the package names,
> rather than the top-level directory names. E.g. only
> com.google.gerrit.httpd.* gets a Jetty dependency; it
> doesn't need to be in gerrit-httpd to suggest that.

Yes, I think you'r right, that would be clear enough, we are
generally defining visibility based on some common level
directory, it should be just as easy to see for developers.

> Also, note that having a simpler overall layout makes
> further refactoring to make package names more sane and
> consistent easier :)

Yes

> Another answer is we can use bazel's visibility
> restrictions to help enforce any convention we decide on.
> If we really strongly feel that Jetty shouldn't be
> visible outside of com.google.gerrit.httpd, then we just
> add this to the //lib:jetty rule:
> visibility = ["//java/com/google/gerrit/httpd:__pkg__"]

This too sounds good, to make it explicit.

> So even if we lose some ability to guess the dependency
> structure at a glance, we can still prevent packages from
> picking up dependencies they shouldn't.

Yes +1 for the whole redesign. Sanity at last?

[Dave Borowitz]

Thanks to David for taking the baton and running with it. I think we can start reviewing/submitting this topic as soon as we cut the 2.15 release branch.

I'd also like to clarify the proposal around test utilities, continuing in the vein of matching the Gerrit layout to what we use internally at Google:

* Test utilities go in the java tree; specifically, tests for subpackage foo go in java/com/google/gerrit/foo/testing

* java_library rules for test utilities get testonly = 1

* Tests for subpackage foo go in javatests/com/google/gerrit/foo

* Tests for test utilities go in javatests/com/google/gerrit/foo/testing

[David Ostrovsky]

Am Dienstag, 12. September 2017 14:49:21 UTC+2 schrieb Dave Borowitz:

Thanks to David for taking the baton and running with it. I think we can start reviewing/submitting this topic as soon as we cut the 2.15 release branch.

Thanks Dave for this proposal. This refactoring was long overdue! Ironically, we removed Maven build

4 years ago, and we are dissolving Maven directory structures only now...

During the work, i re-arranged a couple of packages (I tried to avoid non-intrusive changes,

as the series contains already 41 changes), and reshuffle was much easier to accomplish,

because the question "What top level directory (from 30+) should host that package?" gone.

I'd also like to clarify the proposal around test utilities, continuing in the vein of matching the Gerrit layout to what we use internally at Google:

* Test utilities go in the java tree; specifically, tests for subpackage foo go in java/com/google/gerrit/foo/testing

* java_library rules for test utilities get testonly = 1

* Tests for subpackage foo go in javatests/com/google/gerrit/foo

* Tests for test utilities go in javatests/com/google/gerrit/foo/testing

This would be trivial to do, of course. I haven't looked into impact of this clarification to the current

series, though.

[David Ostrovsky]

Am Dienstag, 12. September 2017 14:49:21 UTC+2 schrieb Dave Borowitz:

Thanks to David for taking the baton and running with it. I think we can start reviewing/submitting this topic as soon as we cut the 2.15 release branch.

I would like to suggest to merge the whole series or a part of it before cutting 2.15 stable branch.

As David Pursehouse pointed out in this comment the CI is blocked for very long time after

the rebase of ca. 50 changes (12+ hours), and beside that, the rebase is not trivial at all.

For big changes, like dissolve gerrit-sever top-level directory, i gave up the rebase attempt and

implemented the move second time.

Another reason to do it before 2.15:

2.14.4 was released today morning. So that 2.14 should be stable by now. For 2.15 I would expect

more activity to port changes from master. After the refactoring of directory structure, this would be

more work and harder to do.

 

I'd also like to clarify the proposal around test utilities, continuing in the vein of matching the Gerrit layout to what we use internally at Google:

* Test utilities go in the java tree; specifically, tests for subpackage foo go in java/com/google/gerrit/foo/testing

* java_library rules for test utilities get testonly = 1

* Tests for subpackage foo go in javatests/com/google/gerrit/foo

* Tests for test utilities go in javatests/com/google/gerrit/foo/testing

Done. I've sent you the changes for review.

[David Pursehouse]

On Thu, Sep 21, 2017 at 5:32 PM David Ostrovsky <david.o...@gmail.com> wrote:

Am Dienstag, 12. September 2017 14:49:21 UTC+2 schrieb Dave Borowitz:

Thanks to David for taking the baton and running with it. I think we can start reviewing/submitting this topic as soon as we cut the 2.15 release branch.

I would like to suggest to merge the whole series or a part of it before cutting 2.15 stable branch.

As David Pursehouse pointed out in this comment the CI is blocked for very long time after

the rebase of ca. 50 changes (12+ hours), and beside that, the rebase is not trivial at all.

For big changes, like dissolve gerrit-sever top-level directory, i gave up the rebase attempt and

implemented the move second time.

Another reason to do it before 2.15:

2.14.4 was released today morning. So that 2.14 should be stable by now. For 2.15 I would expect

more activity to port changes from master. After the refactoring of directory structure, this would be

more work and harder to do.

+1 for doing this before cutting stable-2.15

 

 

I'd also like to clarify the proposal around test utilities, continuing in the vein of matching the Gerrit layout to what we use internally at Google:

* Test utilities go in the java tree; specifically, tests for subpackage foo go in java/com/google/gerrit/foo/testing

* java_library rules for test utilities get testonly = 1

* Tests for subpackage foo go in javatests/com/google/gerrit/foo

* Tests for test utilities go in javatests/com/google/gerrit/foo/testing

Done. I've sent you the changes for review.

--
--
To unsubscribe, email repo-discuss...@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

[Martin Fick]

On Thursday, September 21, 2017 01:32:56 AM David Ostrovsky
wrote:

> Am Dienstag, 12. September 2017 14:49:21 UTC+2 schrieb
Dave Borowitz:
> > Thanks to David for taking the baton and running with it

> > <https://gerrit-review.googlesource.com/q/topic:%22flatt
> > en-directory-structure%22+(status:open%20OR%20status:mer
> > ged)>. I think we can start reviewing/submitting this

> > topic as soon as we cut the 2.15 release branch.
>
> I would like to suggest to merge the whole series or a
> part of it before cutting 2.15 stable branch.
> As David Pursehouse pointed out in this comment

> <https://gerrit-review.googlesource.com/c/gerrit/+/120230/
> 11#message-5f8fb0a975492a4cbc2485300fb98a405fb03794> the

> CI is blocked for very long time after
> the rebase of ca. 50 changes (12+ hours), and beside that,
> the rebase is not trivial at all.
> For big changes, like dissolve gerrit-sever top-level
> directory

> <https://gerrit-review.googlesource.com/c/gerrit/+/122032

> >, i gave up the rebase attempt and
> implemented the move second time.
>
> Another reason to do it before 2.15:
> 2.14.4 was released today morning. So that 2.14 should be
> stable by now. For 2.15 I would expect
> more activity to port changes from master. After the
> refactoring of directory structure, this would be
> more work and harder to do.

-2

As mentioned in another email discussing large changes a few
months ago, the best time to merge stuff like this is after
(likely a few weeks) cutting a new release, not right
before. This ensures that the stable release is stable, and
it should give enough time for immediate bug fixes to still
be easily portable,

[David Ostrovsky]

What is your definition of big changes? This series is a no-op!

We move around thousands of files. c'est tout. Because we

are doing this in small steps, this series contains ca. 50 changes.

Because this project doesn't have enough CI resources, we cannot

handle such huge CI queue and this is really annoying, if contributors

need to wait 12+ hours that their changes are verified. Moreover, talking

about this series, that is immediately outdated and has merge conflicts,

the last changes in the series are not even verified, because (to save CI

resources), the CI flow was restricted to only verify changes that don't have conflicts.

@Luca, can we relax this restriction until this series is merged and

verify changes even with merge conflicts?

[Martin Fick]

On Thursday, September 21, 2017 03:05:07 PM David Ostrovsky

Was that sarcastic? If not, I think you answered your own
question below. :)

> We move around thousands of files. c'est tout. Because we
> are doing this in small steps, this series contains ca. 50
> changes. Because this project doesn't have enough CI
> resources, we cannot handle such huge CI queue and this
> is really annoying, if contributors need to wait 12+
> hours that their changes are verified. Moreover, talking
> about this series, that is immediately outdated and has
> merge conflicts, the last changes in the series are not
> even verified, because (to save CI resources), the CI
> flow was restricted to only verify changes that don't
> have conflicts.

[Luca Milanesio]

Apologies, that was because the GCE nodes were getting selected as "1st choice" and, unfortunately, are much slower than GerritForge nodes :-(

I've rectified the scheduling and now builds are typically triggered with an average delay of 20', that isn't bad at all :-)

I can work with Han-Wen to automate the provisioning of more GCE nodes "on-demand" during the next forthcoming Hackathon so that we can make the 20' wait queue even shorter.

Moreover, talking

about this series, that is immediately outdated and has merge conflicts,

the last changes in the series are not even verified, because (to save CI

resources), the CI flow was restricted to only verify changes that don't have conflicts.

@Luca, can we relax this restriction until this series is merged and

verify changes even with merge conflicts?

I could but how do you build a file with conflicts markers in it? The build would just fail :-(

And even if *by chance* would succeed: what's the value of giving a Verified +1 so something that would *never be mergeable* if not rebased?

[Luca Milanesio]

See at [1] the graph of ETA of a change validation build.

The spikes going up to 1h are the ones related to the GCE nodes, whilst the average ETA when running on a GerritForge node is between 15 and 20'.

[1] https://gerrit-ci.gerritforge.com/job/Gerrit-verifier-change/buildTimeGraph/png

[David Pursehouse]

 

> What is your definition of big changes? This series is a
> no-op!

Was that sarcastic?  If not, I think you answered your own
question below. :)

By "no-op" be means that the series doesn't introduce any functional changes.  It's only rearranging the location of files and adjusting the build rules accordingly.

Any instability caused by this will manifest itself as a build failure due to a rule/dependency getting broken.  It will not change runtime behavior.

The reason we want to do this before cutting stable-2.15 is that if we do it on master *after* stable-2.15, then any commit done on stable-2.15 is very likely to conflict when merged up to master.

Conflicts are already enough when we have a normal level of code restructuring on master due to ongoing development.  I don't want to have to deal with additional conflicts at the scale introduced here (literally almost every file in the repository is getting moved).

[David Ostrovsky]

Sure, my call was to stop trying to merge during CI build in the first place and temporarily

revert this change: [1] (I understand the reason behind this strategy and behind this change).

But, as I said in my previous comment, a series with 50 changes and that is moving almost

every file around is becoming very fast outdated and with skipping conflicting changes CI

strategy, the last changes in the series were not verified at all.

[1] https://gerrit-review.googlesource.com/93655 

[Sven Selberg]

My five öre.

The new directory structure is something that we are going to do and David O. (that have been doing all the heavy lifting) is

currently neck deep in rebase-quagmire, not to mention the CI issues mentioned earlier in this thread.

Why not just get this in ASAP.

IMHO the way to go is to block all submissions to master (qwt-ui and polygerrit excepted)

from a specific date (say monday 25/9). Up until that date try to submit all the open changes that we can get in.

At $DATE:

* No-one submits anything other than changes from "flatten-directory-structure" stack until it is merged

  in its entirety (qwt-ui and polygerrit excepted since from what I can see they're not affected).

* Ask owners of open changes to rebase their changes on top of the "flatten-directory-structure" stack.

* All hands on deck to rebase, review and submit the entire "flatten-directory-structure" stack.

* Sigh of relief

/Sven

[Martin Fick]

On Friday, September 22, 2017 12:50:20 AM David Pursehouse
wrote:

> > > What is your definition of big changes? This series is
> > > a
> > > no-op!
> >
> > Was that sarcastic? If not, I think you answered your
> > own question below. :)
>
> By "no-op" be means that the series doesn't introduce any
> functional changes. It's only rearranging the location
> of files and adjusting the build rules accordingly.
>
> Any instability caused by this will manifest itself as a
> build failure due to a rule/dependency getting broken.
> It will not change runtime behavior.
>
> The reason we want to do this before cutting stable-2.15
> is that if we do it on master *after* stable-2.15, then
> any commit done on stable-2.15 is very likely to conflict
> when merged up to master.

And thus my suggestion of " the best time to merge stuff like
this is after (likely a few weeks) cutting a new release".
This gives the stable release a reasonable amount of time to
have bug fixes applied and still be easily forward ported.

I cannot endorse destabilizing a release to this level to
help make future developments easier. Releases are for
users, not for devs,

[Logan Hanks]

PolyGerrit went through something similar just after 2.14 was released. We migrated all the javascript to ES6, touching most of our lines of code. Fortunately we didn't have to move any files, nor was our code so critical as to need the same sort of strong stability guarantees or as much cherrypicking, but it was a complication.

Perhaps the best time to do this sort of thing is midway between releases, when cherrypicks are rare and there is time before the next release to guarantee stability. I don't think it's reasonable for anyone to try to maintain this series for so long, though. It seems like the question is, can it be put down and picked up again later?

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

[David Ostrovsky]

On Friday, September 22, 2017 at 6:27:20 PM UTC+2, Logan Hanks wrote:

PolyGerrit went through something similar just after 2.14 was released. We migrated all the javascript to ES6, touching most of our lines of code. Fortunately we didn't have to move any files, nor was our code so critical as to need the same sort of strong stability guarantees or as much cherrypicking, but it was a complication.

Thanks for this example. It's easy to see why ES5 => ES6 source code

transformation and "flatten directory structure"- series are something

completely different.

ES5 => ES6 conversion touched every single line in every single JavaScript file.

Moreover, in the same time, because missing ES6 support for all browser atm,

PG introduced transpilation tool chain, added new dependencies on Bazel

rules_closure, that added another complication, because they don't use our home

grown maven_jar() Skylark rule for fetching third party dependencies, but use their

own. (Not related here, but see https://github.com/bazelbuild/bazel/issues/3782).

How can you now formally prove the equivalence of the old ES5 and new ES6 PG
program? You can't. Because hand written ES5 is completely different from ES6

transpiled back to ES5 code, that uses Google's JS runtime.

Conclusion: ES5 => ES6 transformation of the whole code base is very invasive change.

There is no way to formally prove the transformation equivalence. You need to test all

manually, or in case of PG, there are plenty of JS tests.

In contrary, moving Java file around, without changing any line of code, produces exactly the

same byte code. Example:

old location: gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java

new location: java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java

On most recent master, build old gerrit-oauth library:

$ davido@wizball:~/projects/gerrit2 (master %>)$ bazel build gerrit-oauth:oauth

INFO: Found 1 target...

Target //gerrit-oauth:oauth up-to-date:

  bazel-bin/gerrit-oauth/liboauth.jar

Checkout this "no-op" change: from the "flatten directory structure" series, that moves oauth java

files in different location: https://gerrit-review.googlesource.com/121273

R

java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java

0

^^^ As you can see, no changes. Let's build the new library:

gerrit ((2ed234f78d...) %)$ bazel build //java/com/google/gerrit/httpd/auth/oauth:oauth 

INFO: Found 1 target...

Target //java/com/google/gerrit/httpd/auth/oauth:oauth up-to-date:

  bazel-bin/java/com/google/gerrit/httpd/auth/oauth/liboauth.jar

Now let's compare old and new binaries:

davido@wizball:~/projects/gerrit ((2ed234f78d...) %)$ diff ../gerrit2/bazel-bin/gerrit-oauth/liboauth.jar bazel-bin/java/com/google/gerrit/httpd/auth/oauth/liboauth.jar

As you can see, they are the same. Now, all we need to prove is the fact,

that the both libraries are correctly included in the final WAR artifact.

We build headless.war in both trees and grep for liboauth:

New:

davido@wizball:~/projects/gerrit ((2ed234f78d...) %)$ unzip -t bazel-bin/headless.war | grep liboauth

    testing: WEB-INF/lib/com_google_gerrit_httpd_auth_oauth_liboauth.jar   OK

Old:

davido@wizball:~/projects/gerrit2 (master %>)$ unzip -t bazel-bin/headless.war | grep liboauth

    testing: WEB-INF/lib/gerrit-oauth-liboauth.jar   OK

As you can see we have now a formal proof of the equivalence of move of oauth package from one

location to another. That's what I meant with a "no-op" change. (It worth noting, that we should check

Eclipse project generation with changed directory structure as well, but this is something that developers

care about and not the users; so we don't bother to check but this works too).

Conclusion: Dissolving of top-level directory structure is a no-op change that produces the same binary.

A no-op change can always be done in the release cycle: before, after and in the middle of release.

Perhaps the best time to do this sort of thing is midway between releases, when cherrypicks are rare and there is time before the next release to guarantee stability. I don't think it's reasonable for anyone to try to maintain this series for so long, though. It seems like the question is, can it be put down and picked up again later?

Yes, we can do that, too.

[Martin Fick]

On Friday, September 22, 2017 12:20:00 PM David Ostrovsky
wrote:

> Conclusion: Dissolving of top-level directory structure is
> a no-op change that produces the same binary.

This is very good news.

You mentioned developer tools integration (eclipse), but
what else should we be concerned with? Release tools?
Plugins, in and out of tree? Plugins that still use maven,
buck (I suspect there are still many)?

[thomasmu...@yahoo.com]

Most repos were converted to bazel on gerrit-review, not sure about in the wild though outside of gerrit-review.

[David Ostrovsky]

On Friday, September 22, 2017 at 10:05:47 PM UTC+2, MartinFick wrote:

On Friday, September 22, 2017 12:20:00 PM David Ostrovsky
wrote:
> Conclusion: Dissolving of top-level directory structure is
> a no-op change that produces the same binary.

This is very good news.

You mentioned developer tools integration (eclipse), but
what else should we be concerned with?  Release tools?

Right, tools/maven/api.sh was adjusted, and for the same reasons, explained

in my previous comment, the plugin API binaries are not changed after ther

refactoring.

Plugins, in and out of tree?  Plugins that still use maven,
buck (I suspect there are still many)?

The same Maven Central artifacts with exactly the same content are still published

after the refactoring:

* gerrit-extension-api

* gerrit-plugin-api

* gerrit-plugin-gwtui

* gerrit-acceptance-framework

That why all standalone plugin builds: Bucklets, Bazlets, Maven, Buck or 

<put your favorite build tool chain here> driven build would not notice any change.

In Gerrit tree build mode, would still work given that plugin is using plugin API abstraction

(all plugins I aware of do use it):

Wrong:

    [...]

    deps = [

        "//gerrit-acceptance-framework:lib",

        "//gerrit-plugin-api:lib",

    ],

    [...]

Right:

    [...]

    deps = PLUGIN_TEST_DEPS + PLUGIN_DEPS + [

    [...]

The change, that dissolves gerrit-plugin-api top-level directory, is adjusting

the constants, obviously, so that the in gerrit tree build mode would just continue

to work: [1], and the plugins would not need to change anything.

Surprisingly enough, the only (potential) plugin breakage I've found

so far with the changed directory structure is replication plugin. But, this

is only because it wasn't using the plugin API abstraction and accessed the

actual plugin API and acceptance framework artifacts directly. This was

wrong anyway and was already fixed and merged, even before "flatten

directory structure" - series is merged: [2].

* [1] https://gerrit-review.googlesource.com/#/c/gerrit/+/121972/8/tools/bzl/plugin.bzl@12

* [2] https://gerrit-review.googlesource.com/#/c/plugins/replication/+/121892/1/BUILD@25

[Dave Borowitz]

Not going to comment on the rebase hell that David is going through (sorry) or the potential for instability leading up to a release (which sounds like a non-issue given that it produces the same war).

From my (Google's) perspective, a bigger blocker than the 2.15 cut is redoing our internal build. But I think it might not actually be that bad, and I will take a crack at it today or tomorrow morning.

On Thu, Sep 21, 2017 at 10:32 AM, David Ostrovsky <david.o...@gmail.com> wrote:

Am Dienstag, 12. September 2017 14:49:21 UTC+2 schrieb Dave Borowitz:

Thanks to David for taking the baton and running with it. I think we can start reviewing/submitting this topic as soon as we cut the 2.15 release branch.

I would like to suggest to merge the whole series or a part of it before cutting 2.15 stable branch.

As David Pursehouse pointed out in this comment the CI is blocked for very long time after

the rebase of ca. 50 changes (12+ hours), and beside that, the rebase is not trivial at all.

For big changes, like dissolve gerrit-sever top-level directory, i gave up the rebase attempt and

implemented the move second time.

Another reason to do it before 2.15:

2.14.4 was released today morning. So that 2.14 should be stable by now. For 2.15 I would expect

more activity to port changes from master. After the refactoring of directory structure, this would be

more work and harder to do.

Is that really true? Shouldn't `git cherry-pick` work across moves? Can you try, as an experiment, making a content change on top of your series (simulating post-2.15 master) and cherry-picking onto master (simulating stable-2.15)?

 

 

I'd also like to clarify the proposal around test utilities, continuing in the vein of matching the Gerrit layout to what we use internally at Google:

* Test utilities go in the java tree; specifically, tests for subpackage foo go in java/com/google/gerrit/foo/testing

* java_library rules for test utilities get testonly = 1

* Tests for subpackage foo go in javatests/com/google/gerrit/foo

* Tests for test utilities go in javatests/com/google/gerrit/foo/testing

Done. I've sent you the changes for review.

[David Ostrovsky]

On Sunday, September 24, 2017 at 9:08:08 PM UTC+2, Dave Borowitz wrote:

Not going to comment on the rebase hell that David is going through (sorry) or the potential for instability leading up to a release (which sounds like a non-issue given that it produces the same war).

From my (Google's) perspective, a bigger blocker than the 2.15 cut is redoing our internal build. But I think it might not actually be that bad, and I will take a crack at it today or tomorrow morning.

Beside the aspect of redoing your internal build, can someone from Gerrit

team @Google start to review the series? I asked Damien Martin-Guillerez

from Bazel team to help with the review of this series, but he deferred the

review process to Gerrit team @Google.

Another option is to do what Logan suggested, abandon this series for now,

and proceed with it later (I have a long vacation starting at the end of the

next week).

 

On Thu, Sep 21, 2017 at 10:32 AM, David Ostrovsky <david.o...@gmail.com> wrote:

Am Dienstag, 12. September 2017 14:49:21 UTC+2 schrieb Dave Borowitz:

Thanks to David for taking the baton and running with it. I think we can start reviewing/submitting this topic as soon as we cut the 2.15 release branch.

I would like to suggest to merge the whole series or a part of it before cutting 2.15 stable branch.

As David Pursehouse pointed out in this comment the CI is blocked for very long time after

the rebase of ca. 50 changes (12+ hours), and beside that, the rebase is not trivial at all.

For big changes, like dissolve gerrit-sever top-level directory, i gave up the rebase attempt and

implemented the move second time.

Another reason to do it before 2.15:

2.14.4 was released today morning. So that 2.14 should be stable by now. For 2.15 I would expect

more activity to port changes from master. After the refactoring of directory structure, this would be

more work and harder to do.

Is that really true? Shouldn't `git cherry-pick` work across moves? Can you try, as an experiment, making a content change on top of your series (simulating post-2.15 master) and cherry-picking onto master (simulating stable-2.15)?

Indeed, you are right. I tried to cherry-pick one trivial change: [1]

that I applied on top of this refactoring series, and cherry-pick on

master (with old directory structure) just worked.

* [1] http://paste.openstack.org/show/621809

[Dave Borowitz]

On Sun, Sep 24, 2017 at 10:38 PM, David Ostrovsky <david.o...@gmail.com> wrote:

On Sunday, September 24, 2017 at 9:08:08 PM UTC+2, Dave Borowitz wrote:

Not going to comment on the rebase hell that David is going through (sorry) or the potential for instability leading up to a release (which sounds like a non-issue given that it produces the same war).

From my (Google's) perspective, a bigger blocker than the 2.15 cut is redoing our internal build. But I think it might not actually be that bad, and I will take a crack at it today or tomorrow morning.

Beside the aspect of redoing your internal build, can someone from Gerrit

team @Google start to review the series?

I will review it as part of redoing the internal build.

To be completely honest, what I was going to do was just start with the BUILD files in the tree at the tip of the series, and tweak them in-place to handle the bazel->blaze conversion. So I'll be reading the files closely anyway.

 

I asked Damien Martin-Guillerez

from Bazel team to help with the review of this series, but he deferred the

review process to Gerrit team @Google.

Another option is to do what Logan suggested, abandon this series for now,

and proceed with it later (I have a long vacation starting at the end of the

next week).

Would you be offended if we worked on this at the hackathon? Maybe you'll come back from your vacation and it will all be merged ;)

 

 

On Thu, Sep 21, 2017 at 10:32 AM, David Ostrovsky <david.o...@gmail.com> wrote:

Am Dienstag, 12. September 2017 14:49:21 UTC+2 schrieb Dave Borowitz:

Thanks to David for taking the baton and running with it. I think we can start reviewing/submitting this topic as soon as we cut the 2.15 release branch.

I would like to suggest to merge the whole series or a part of it before cutting 2.15 stable branch.

As David Pursehouse pointed out in this comment the CI is blocked for very long time after

the rebase of ca. 50 changes (12+ hours), and beside that, the rebase is not trivial at all.

For big changes, like dissolve gerrit-sever top-level directory, i gave up the rebase attempt and

implemented the move second time.

Another reason to do it before 2.15:

2.14.4 was released today morning. So that 2.14 should be stable by now. For 2.15 I would expect

more activity to port changes from master. After the refactoring of directory structure, this would be

more work and harder to do.

Is that really true? Shouldn't `git cherry-pick` work across moves? Can you try, as an experiment, making a content change on top of your series (simulating post-2.15 master) and cherry-picking onto master (simulating stable-2.15)?

Indeed, you are right. I tried to cherry-pick one trivial change: [1]

that I applied on top of this refactoring series, and cherry-pick on

master (with old directory structure) just worked.

* [1] http://paste.openstack.org/show/621809

[David Ostrovsky]

On Sunday, September 24, 2017 at 10:54:32 PM UTC+2, Dave Borowitz wrote:

On Sun, Sep 24, 2017 at 10:38 PM, David Ostrovsky <david.o...@gmail.com> wrote:

On Sunday, September 24, 2017 at 9:08:08 PM UTC+2, Dave Borowitz wrote:

Not going to comment on the rebase hell that David is going through (sorry) or the potential for instability leading up to a release (which sounds like a non-issue given that it produces the same war).

From my (Google's) perspective, a bigger blocker than the 2.15 cut is redoing our internal build. But I think it might not actually be that bad, and I will take a crack at it today or tomorrow morning.

Beside the aspect of redoing your internal build, can someone from Gerrit

team @Google start to review the series?

I will review it as part of redoing the internal build.

To be completely honest, what I was going to do was just start with the BUILD files in the tree at the tip of the series, and tweak them in-place to handle the bazel->blaze conversion. So I'll be reading the files closely anyway.

 

I asked Damien Martin-Guillerez

from Bazel team to help with the review of this series, but he deferred the

review process to Gerrit team @Google.

Another option is to do what Logan suggested, abandon this series for now,

and proceed with it later (I have a long vacation starting at the end of the

next week).

Would you be offended if we worked on this at the hackathon?

Of course not ;-)

 

Maybe you'll come back from your vacation and it will all be merged ;)

+1.

[Dave Borowitz]

On Sun, Sep 24, 2017 at 3:07 PM, Dave Borowitz <dbor...@google.com> wrote:

Not going to comment on the rebase hell that David is going through (sorry) or the potential for instability leading up to a release (which sounds like a non-issue given that it produces the same war).

From my (Google's) perspective, a bigger blocker than the 2.15 cut is redoing our internal build. But I think it might not actually be that bad, and I will take a crack at it today or tomorrow morning.

I spent a couple hours on our internal build today and made good progress. I think we should be able to complete this during the hackathon.

I need to take another look at the blocking issues for 2.15 and see whether it's feasible to get it all done during the week before I leave for London.

[David Ostrovsky]

On Monday, September 25, 2017 at 11:54:20 PM UTC+2, Dave Borowitz wrote:

On Sun, Sep 24, 2017 at 3:07 PM, Dave Borowitz <dbor...@google.com> wrote:

Not going to comment on the rebase hell that David is going through (sorry) or the potential for instability leading up to a release (which sounds like a non-issue given that it produces the same war).

From my (Google's) perspective, a bigger blocker than the 2.15 cut is redoing our internal build. But I think it might not actually be that bad, and I will take a crack at it today or tomorrow morning.

I spent a couple hours on our internal build today and made good progress. I think we should be able to complete this during the hackathon.

I need to take another look at the blocking issues for 2.15 and see whether it's feasible to get it all done during the week before I leave for London.

SGTM. I wonder if you could distribute the work among different developers on your team.

Gerrit/Git Team @Google is not a one man show any more. During my work on Bazel

migration I got very valuable input from Masaya, Jonathan and Han-Wen. I wonder if they

could help with review of dissolving directory structure series and redoing your internal

build?

[Dave Borowitz]

I ran into a nontrivial issue that I commented on here:

https://gerrit-review.googlesource.com/c/gerrit/+/121151

I think I don't have the further time to work on this before the hackathon, unfortunately. It's not looking very good for getting in 2.15.

[David Ostrovsky]

On Tuesday, September 26, 2017 at 11:11:35 PM UTC+2, Dave Borowitz wrote:

I ran into a nontrivial issue that I commented on here:

https://gerrit-review.googlesource.com/c/gerrit/+/121151

I don't think it is an issue. I answered your comment.

 

I think I don't have the further time to work on this before the hackathon, unfortunately.

Thanks for the rebase, Dave!

 

It's not looking very good for getting in 2.15.

As you pointed out, cherry-picking commits after this series is merged to the older

gerrit tree is a non-issue. I'm fine to merge this series after 2.15-rc0 is cut, then.

[Edwin Kempin]

There is one thing that I noticed which the new package structure that I find a bit annoying.

Let's say I have executed all tests and all tests passed, then I make a change in one test, e.g. 'DeleteBranchesIT', and rerun the tests.

In stable-2.15 (old package structure) it only executes

'//gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project:rest_project'

and for all other tests it takes the cached 'PASSED' result.

However in master (new package structure) it re-executes all the tests :(

[Dave Borowitz]

I don't think that's a result of rearranging the directory structure. The only test of which DeleteBranchesIT.java is a src is //javatests/com/google/gerrit/acceptance/rest/project:rest_project. The package name is different, but the BUILD file is the same:

https://gerrit-review.googlesource.com/c/gerrit/+/121574/15/javatests/com/google/gerrit/acceptance/rest/project/BUILD

I think something else is wrong with either our dependency graph or your bazel settings.

[דוד אוסטרובסקי‎]

2017-11-10 15:07 GMT+01:00 Dave Borowitz <dbor...@google.com>:

I don't think that's a result of rearranging the directory structure. The only test of which DeleteBranchesIT.java is a src is //javatests/com/google/gerrit/acceptance/rest/project:rest_project. The package name is different, but the BUILD file is the same:

https://gerrit-review.googlesource.com/c/gerrit/+/121574/15/javatests/com/google/gerrit/acceptance/rest/project/BUILD

I think something else is wrong with either our dependency graph or your bazel settings.

Right. Moreover, the dependency graph remained (almost) the same after the refactoring:

//javatests/com/google/gerrit/acceptance/rest/project:rest_project => acceptance/framework => gerrit/server => rest

So I fail to see (I do not have access to my machine atm), how changing sources from project:rest_project

would cause re-build or re-test of the entire world.

[Edwin Kempin]

OK. Would be nice to know if I'm the only one seeing this issue or if it's reproducible on other machines.

 

[דוד אוסטרובסקי‎]

I was able to reproduce it. As expected, that problem has nothing to do

with re-arranging of directory structure series.

The problem you've discovered is a regression of: [1], that was not cherry

picked to stable-2.15. So, as always, it is a good idea to report an issue in

issue tracker.

Why this change has such consequences?

This has something to do with how stamping is implemented in Bazel.

- Every time you run bazel build foo (or bazel test bar)

   bazel workspace status command is unconditionaly executed

- cat tools/bazel.rc 

build --workspace_status_command=./tools/workspace-status.sh

- in the end this line from ./tools/workspace-status.sh is executed

    git describe --always --match "v[0-9].*" --dirty

and the output is written to bazel-out/volatile-status.txt and bazel-out/stable-status.txt

- Because your git tree was clean, and you've changed one file,

the content of the version was changed to something like:

v2.15-rc2-892-g22a6c6e2b3-dirty

- As the consequence, //:version is rebuilt

- As the consequence, because since [1] 

java/com/google/gerrit/common:server depends on //:version,

the common/server.jar was rebuilt and got new resource file:

cat ./com/google/gerrit/common/Version

v2.15-rc2-892-g22a6c6e2b3-dirty

- As the consequence, all tests are re-executed.

There are two major use cases to consider here:

Scenario I: (You hit that one)

a) Start from clean git tree

b) run bazel test //... => all tests are executed

c) touch no matter what file in the tree

d) re-run bazel test //... => all tests are re-executed because the

version changed from say v2.15-rc2-892-g22a6c6e2b3 to v2.15-rc2-892-g22a6c6e2b3-dirty

Scenario II:

a*) start from non clean tree, say version is: v2.15-rc2-892-g22a6c6e2b3-dirty

b*) run bazel test //... => all tests are executed

c*) change javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java file

d*) re-run bazel test //... => only one test rule is re-executed:

//javatests/com/google/gerrit/acceptance/rest/project:rest_project

I think, we should re-consider: [1] implementation and try to move the version tests

in different place in the dependency graph, so that Scenario I only affects very few

tests.

Another option would be to change tools/workspace-status.sh to always generate

say "dev" string and not consult `git describe ...` in development tree.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/138791

[Edwin Kempin]

On Fri, Nov 10, 2017 at 10:13 PM, דוד אוסטרובסקי <david.o...@gmail.com> wrote:

2017-11-10 15:54 GMT+01:00 Edwin Kempin <eke...@google.com>:

On Fri, Nov 10, 2017 at 3:32 PM, דוד אוסטרובסקי <david.o...@gmail.com> wrote:

2017-11-10 15:07 GMT+01:00 Dave Borowitz <dbor...@google.com>:

I don't think that's a result of rearranging the directory structure. The only test of which DeleteBranchesIT.java is a src is //javatests/com/google/gerrit/acceptance/rest/project:rest_project. The package name is different, but the BUILD file is the same:

https://gerrit-review.googlesource.com/c/gerrit/+/121574/15/javatests/com/google/gerrit/acceptance/rest/project/BUILD

I think something else is wrong with either our dependency graph or your bazel settings.

Right. Moreover, the dependency graph remained (almost) the same after the refactoring:

//javatests/com/google/gerrit/acceptance/rest/project:rest_project => acceptance/framework => gerrit/server => rest

So I fail to see (I do not have access to my machine atm), how changing sources from project:rest_project

would cause re-build or re-test of the entire world.

OK. Would be nice to know if I'm the only one seeing this issue or if it's reproducible on other machines.

I was able to reproduce it. As expected, that problem has nothing to do

with re-arranging of directory structure series.

Thanks a lot for looking into this.

 

The problem you've discovered is a regression of: [1], that was not cherry

picked to stable-2.15. So, as always, it is a good idea to report an issue in

issue tracker.

Yeah, it should have been an issue. Should we still create one?

 

Why this change has such consequences?

This has something to do with how stamping is implemented in Bazel.

- Every time you run bazel build foo (or bazel test bar)

   bazel workspace status command is unconditionaly executed

- cat tools/bazel.rc 

build --workspace_status_command=./tools/workspace-status.sh

- in the end this line from ./tools/workspace-status.sh is executed

    git describe --always --match "v[0-9].*" --dirty

and the output is written to bazel-out/volatile-status.txt and bazel-out/stable-status.txt

- Because your git tree was clean, and you've changed one file,

the content of the version was changed to something like:

v2.15-rc2-892-g22a6c6e2b3-dirty

- As the consequence, //:version is rebuilt

- As the consequence, because since [1] 

java/com/google/gerrit/common:server depends on //:version,

the common/server.jar was rebuilt and got new resource file:

cat ./com/google/gerrit/common/Version

v2.15-rc2-892-g22a6c6e2b3-dirty

- As the consequence, all tests are re-executed.

Interesting. Thanks for finding the root cause.

 

There are two major use cases to consider here:

Scenario I: (You hit that one)

a) Start from clean git tree

b) run bazel test //... => all tests are executed

c) touch no matter what file in the tree

d) re-run bazel test //... => all tests are re-executed because the

version changed from say v2.15-rc2-892-g22a6c6e2b3 to v2.15-rc2-892-g22a6c6e2b3-dirty

Scenario II:

a*) start from non clean tree, say version is: v2.15-rc2-892-g22a6c6e2b3-dirty

b*) run bazel test //... => all tests are executed

c*) change javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java file

d*) re-run bazel test //... => only one test rule is re-executed:

//javatests/com/google/gerrit/acceptance/rest/project:rest_project

I think, we should re-consider: [1] implementation and try to move the version tests

in different place in the dependency graph, so that Scenario I only affects very few

tests.

Another option would be to change tools/workspace-status.sh to always generate

say "dev" string and not consult `git describe ...` in development tree.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/138791