Migrate to JDK 11+ #961

fzdy1914 · 2019-02-08T13:52:34Z

Fixes #953

CanIHasReview-bot · 2019-02-08T13:52:35Z

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

fzdy1914 · 2019-02-20T14:00:09Z

@pyokagan Ready to review.

pyokagan · 2019-02-20T15:04:45Z

@fzdy1914

@pyokagan Ready to review.

Please use the link in #961 (comment) to submit for review. Thanks.

CanIHasReview-bot · 2019-02-20T18:05:22Z

v1

@fzdy1914 submitted v1 for review.

[v1 1/14] build.gradle: add javafx runtime dependency
[v1 2/14] change the entry point of addressbook
[v1 3/14] build.gradle: add javafx runtime dependency for other platforms
[v1 4/14] build.gradle: fix checkstyle plugin failure
[v1 5/14] build.gradle: bump target and source compatibility to JDK 11
[v1 6/14] Gradle wrapper: upgrade to Gradle 5.0
[v1 7/14] build.gradle: remove wrapper task
[v1 8/14] Build.gradle: upgrade shadow plugin to 4.0.4
[v1 9/14] build.gradle: upgrade testFX dependency to 4.0.15-alpha
[v1 10/14] build.gradle: upgrade monocle dependency to jdk-11+26
[v1 11/14] build.gradle: update deprecated code in coverage plugin
[v1 12/14] update CI config
[v1 13/14] disable TLS v1.3 protocol
[v1 14/14] build.gradle: remove 'prism.order' property for headless task.

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/961/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan · 2019-02-22T05:56:56Z

@fzdy1914 Thanks for splitting them up into commits. I now get a nice summarized list of what you are trying to do, which helps me a lot.

I was going to do an in-depth review starting from [1/14], but then I realized that there are a few high-level issues that we should probably sort out first:

Split out Gradle/TestFX upgrading-related commits into a separate PR

As mentioned somewhere else, it is a good-enough reason on its own for us to bump up our Gradle and TestFX versions to keep in pace with their development. The reason why we don't blindly do that is to ensure that we don't introduce regressions that could have been easily avoided (e.g. by reading their release notes), so perhaps you may wish to glance through the release notes/upgrade notes of Gradle/TestFX to see if there is anything that stands out that we might wish to take note of.

So, I recommend that we split out these two PRs from this PR:
1. Upgrade to Gradle 5.0
  
  This would contain the following commits (preferably in the following order):
  1. [v1 7/14] build.gradle: remove wrapper task
  2. [v1 8/14] Build.gradle: upgrade shadow plugin to 4.0.4
  3. [v1 6/14] Gradle wrapper: upgrade to Gradle 5.0
  4. [v1 11/14] build.gradle: update deprecated code in coverage plugin
2. Update TestFX dependency
  
  Which would contain [v1 9/14] build.gradle: upgrade testFX dependency to 4.0.15-alpha
This also ensures that you can quickly get some PRs merged as well :-)
Making the breaking JDK 8 -> JDK 11 change clear inside the PR

As you are probably aware, we can't "break" the project in-between commits. We currently require JDK 8. However, it seems that somewhere around [5/14] you start to require JDK 11. In that case, this must be made very clear in the commit. Let's say you make [5/14] the breaking commit, then commits before [5/14] must work with JDK 8, while commits after [5/14] must work with JDK 11. [5/14] must make all the other necessary changes such as updating our User Guide / Developer Guide to say that we require JDK 11. This includes CI as well, it seems that the CI is broken somewhere during the PR and then fixed in [12/14]? The CI configuration should be updated in [5/14] itself.

Anyway, this probably requires a slight re-arrangement of commits, so please do that and then I'll take a good look at v2.

pyokagan · 2019-02-22T05:58:08Z

It seems the latest gradle version is v5.2.1 so we should probably upgrade to that.

pyokagan · 2019-02-22T06:18:21Z

2\. Making the breaking JDK 8 -> JDK 11 change clear inside the PR

@fzdy1914 No worries if you can't make the build completely not break. The main goal is to designate a single commit as the single "breaking change commit" (that requires devs to now use JDK 11), and we can then work out the details after that.

fzdy1914 · 2019-02-28T12:50:29Z

@pyokagan

so perhaps you may wish to glance through the release notes/upgrade notes of Gradle/TestFX to see if there is anything that stands out that we might wish to take note of.

You have mentioned that I need to go through the release note. Do you mean that I need to take note on what side effects may occur when upgrading Gradle?

pyokagan · 2019-02-28T13:27:53Z

@fzdy1914

You have mentioned that I need to go through the release note. Do you mean that I need to take note on what side effects may occur when upgrading Gradle?

Yes.

fzdy1914 · 2019-02-28T14:24:49Z

@fzdy1914 No worries if you can't make the build completely not break. The main goal is to designate a single commit as the single "breaking change commit" (that requires devs to now use JDK 11), and we can then work out the details after that.

I will try to re-order the commits to minimize the broken commits first.

fzdy1914 · 2019-02-28T15:31:54Z

@pyokagan Can you help with coverall failure? Or I have to add the relative test to it?

CanIHasReview-bot · 2019-03-13T16:48:41Z

v2

@fzdy1914 submitted v2 for review.

[v2 1/7] build.gradle: fix checkstyle plugin failure
[v2 2/7] disable TLS v1.3 protocol
[v2 3/7] build.gradle: add javafx runtime dependency
[v2 4/7] change the entry point of addressbook
[v2 5/7] build.gradle: add javafx runtime dependency for other platforms
[v2 6/7] build.gradle: remove 'prism.order' property for headless task.
[v2 7/7] Upgrade to JDK 11

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/961/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan

[1/7]

build.gradle: fix checkstyle plugin failure

The `checkstyle` plugin of Gradle fails in JDK11.

OK, I can reproduce the error, although it is extremely verbose and uninformative so
it is probably reasonable not to paste it verbatim into the commit
message.

> Unable to create Root Module: config {/home/pyokagan/pyk/addressbook-level4/config/checkstyle/checkstyle.xml}, classpath {/home/pyokagan/pyk/addressbook-level4/build/classes/java/main:/home/pyokagan/pyk/addressbook-level4/build/resources/main:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-fxml/11/e7a757b580fc3e2e1a121768606c7836eef418c/javafx-fxml-11-linux.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-web/11/5f50385bc156b2e3bd478f9fded919f9d8ae8801/javafx-web-11-linux.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-controls/11/794488e3b1f4635d9a1b842bdc872a5eb8fd54ca/javafx-controls-11-linux.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-controls/11/58d961774262ec972bf304e16c154a8e18c2050b/javafx-controls-11.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-media/11/f65e90798210a9ff1cad3e7686af8c1db138477f/javafx-media-11-linux.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-media/11/586c51942ea38145727e345387b6b7cd53de0b33/javafx-media-11.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-graphics/11/4ab9edd8d481a420044c473fdb5718ccdd35c836/javafx-graphics-11-linux.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-graphics/11/a736dd079047ec0b72b8c4970842a5c5e0c19f2f/javafx-graphics-11.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-base/11/f8d1ced6047b010f1e3bb92dc060862179ce5897/javafx-base-11-linux.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-base/11/9fcd3e8e3227ec97bf503f7991fad1f3b14d005/javafx-base-11.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/com.fasterxml.jackson.datatype/jackson-datatype-jsr310/2.7.4/390dbf17d4eb29a6157c6b9dcd9c93e7acac714/jackson-datatype-jsr310-2.7.4.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/com.fasterxml.jackson.core/jackson-databind/2.7.4/1e9c6f3659644aeac84872c3b62d8e363bf4c96d/jackson-databind-2.7.4.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/com.fasterxml.jackson.core/jackson-annotations/2.7.0/19f42c154ffc689f40a77613bc32caeb17d744e3/jackson-annotations-2.7.0.jar:/home/pyokagan/.gradle/caches/modules-2/files-2.1/com.fasterxml.jackson.core/jackson-core/2.7.4/b8f38a249116b66d804a5ca2b14a3459b7913a94/jackson-core-2.7.4.jar}.

The main reason is that, in JDK11, the `user.dir` cannot be reset by Gradle.[1]
So, checkstyle plugin is unable to locate the suppressions file correctly.

Sounds good.

By the way, the lines in the commit message exceed 72 characters.

Also, the `FileContentHolder` module are deprecated[2].

This seems like an entirely separate problem that should be in a
separate commit.

Let's add ` config_loc` variable suggested by Gradle to solve the problem.[1]

[1] https://github.com/gradle/gradle/issues/8286
[2] https://github.com/checkstyle/checkstyle/issues/5187

build.gradle

pyokagan

[3/7]

build.gradle: add javafx runtime dependency

JavaFX is not distributed with Oracle JDK any more from JDK11 onwards [1].

This line narrowly exceeds 72 characters.

Our code uses javafx as our client platform. So it is unable to be compiled
in JDK11 anymore.

As we are moving to JDK11, let’s add javafx runtime dependency to gradle.

Meanwhile, the dependency provided are platform specific. Let’s use the
SystemUtils api provided by Apache [2] to specify the version of javafx
dependency.

I forgot to ask last time: why did you decide to use the SystemUtils API
in the end?

[1]https://blogs.oracle.com/java-platform-group/the-future-of-javafx-
and-other-java-client-roadmap-updates
[2]http://commons.apache.org/proper/commons-lang/javadocs/api-release/
index.html

URLs are not prose and should not be line wrapped.

build.gradle

After adding the javafx runtime dependency, addressbook is still unable to run in a JDK11 environment. It gives an error of below: Error: JavaFX runtime components are missing, and are required to run this application This error comes from sun.launcher.LauncherHelper in the java.base module. The reason for this is that the MainApp extends Application and has a main method. If that is the case, the LauncherHelper will check for the javafx.graphics module to be present as a named module. If that module is not present, the launch is aborted. Hence, having the JavaFX libraries as jars on the classpath is not allowed in this case [1]. This is more like a JDK 11 problem which cannot be solved elegantly. One simple workaround is to have a separate main class that doesn't extend Application. Hence it doesn't do the check on javafx.graphics module, and when the required jars are on the classpath, it works fine. Let's add another main class to be the new entry point of addressbook to solve this problem [2]. [1] http://mail.openjdk.java.net/pipermail/openjfx-dev/2018-June/021977.html [2] https://stackoverflow.com/questions/52653836/maven-shade-javafx-runtime-components-are-missing/52654791#52654791

CanIHasReview-bot · 2019-05-03T03:18:12Z

v11

@fzdy1914 submitted v11 for review.

[v11 1/7] build.gradle: fix checkstyle plugin failure
[v11 2/7] build.gradle: add platform-specific javafx runtime dependencies
[v11 3/7] Change the entry point of addressbook
[v11 4/7] build.gradle: add javafx runtime dependency for all platforms
[v11 5/7] Workaround headless test failure on Windows OS
[v11 6/7] Workaround coveralls upload failure on Travis CI
[v11 7/7] Upgrade to JDK 11

(📚 Archive) (📈 Interdiff between v10 and v11) (📈 Range-Diff between v10 and v11)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/961/11/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan

Why was the commit message for [4/7] completely removed?

We have added the platform-specific javafx runtime dependencies, so the addressbook is able to run locally. However, the jar file generated on one OS cannot run on other OS. The reason is that, after detecting the OS, javafx will try to load platform-specific classes dynamically. Without corresponding javafx dependencies for that other OS, the class loading process will fail. Let's add the javafx runtime dependency for all platforms (MacOS, Window, Linux) so the jar file generated on one OS is able to run in other OS [1]. Following are some key facts that guarantee the solution to work: * Gradle places all dependency JARs on the classpath and the order is not defined by Gradle. * When shadowJar encounters duplicate classes with same package name and same class name, it picks the first one it sees. ShadowJar processes dependency JARs according to their order in classpath. * The platform-specific JARs for different OSs can define the same classes. (e.g. javafx-graphics-11-win.jar and javafx-graphics-11-linux.jar both contain javafx/scene/control/TreeView.class). However, via diffing the contents of the JARs, we have verified that if two JARs define the same class, the class files are also identical to each other. So, it doesn't matter if the TreeView.class from javafx-graphics-11-win.jar or the TreeView.class from javafx-graphics-11-linux.jar is loaded. As such, no matter what order Gradle places JavaFX's JARs on the classpath, we can run and distribute addressbook properly. [1] https://stackoverflow.com/questions/52653836/maven-shade-javafx-runtime-components-are-missing/52654791#52654791

For headless test task, 'prism.order' property is used to choose the graph renderer to use. Currently, we specify this property to be 'sw'. However, this property triggers a bug of openjdk-jfx with headless mode [1]. This property will cause Java Runtime Error for Windows OS including AppVeyor: # A fatal error has been detected by the Java Runtime Environment: # # EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x00007ffd95b64879, pid=1476, tid=2640 # # JRE version: OpenJDK Runtime Environment (11.0.1+13) (build 11.0.1+13) # Java VM: OpenJDK 64-Bit Server VM (11.0.1+13, mixed mode, tiered, compressed oops, g1 gc, windows-amd64) # Problematic frame: # C [javafx_font.dll+0x4879] This bug has been identified and will be fixed in future release [2]. There is a workaround suggested which adds static initialization blocks to load required library before any FxToolkit code [3]. Java will initialize base classes first before classes of instance members [4] [5]. For all GUI tests, they uses GuiUnitTest or AddressBookSystemTest as their base class. Let's add the static initialization blocks to these two classes to solve the problem. [1] javafxports/openjdk-jfx#66 [2] javafxports/openjdk-jfx#66 (comment) [3] javafxports/openjdk-jfx#66 (comment) [4] https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.4 [5] https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.5

Our Travis CI builds are configured to run the Gradle `coveralls` task to upload test coverage information to coveralls.io. However, when our Travis CI builds are also configured to use JDK 11, the following error occurs when running the `coveralls` task: > Task :coveralls FAILED service name: travis-ci service job id: 520624518 repo token: null FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':coveralls'. > javax.net.ssl.SSLProtocolException: Connection reset by peer (Write failed) This is because JDK 11 implements support for TLS v1.3 [1], and will attempt to use it if the server also supports it (coveralls.io does). However, a bug [2] in its TLS v1.3 implementation causes it to send invalid data to coveralls.io. When coveralls.io receives this invalid data, it terminates the connection, hence the error occurs. Let's workaround this problem by disabling TLS v1.3 support in the JDK. This workaround should be fine for now, since not many servers support TLS v1.3 yet as the TLS v1.3 spec was only published on Aug 2018 [3]. Furthermore, JDK developers have already acknowledged the bug [2] and targeted its fix for JDK 13 (i.e. in the near future). [1] http://openjdk.java.net/jeps/332 [2] https://bugs.openjdk.java.net/browse/JDK-8221253 [3] https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_1.3 Co-authored-by: Paul Tan <[email protected]>

We currently advertise that we support "JDK 8". However, the public updates of Java SE 8 for personal users will end soon [1]. JDK 11 is the next Long-Term-Support (LTS) release after JDK 8 [1]. It is better for us to keep updated with the latest release of JDK. Let's update our target JDK to version 11, with the following steps: * We use openjfx-monocle version jdk-11+26 since that is the latest version of openjfx-monocle that supports JDK 11 [2]. * We bump the target and source compatibility of Gradle to JDK11. * We update Travis and AppVeyor configs to use JDK11 as runtime environment. * We remove the add-on in Travis config because it is redundant for JDK 11 [3]. * We make it clear in the User Guide / Developer Guide that we only support JDK 11 and above (not JDK 8, 9, 10). [1] https://www.oracle.com/technetwork/java/java-se-support-roadmap.html [2] https://github.com/TestFX/Monocle [3] https://docs.travis-ci.com/user/languages/java/#using-java-10-and-later

fzdy1914 · 2019-05-03T07:39:17Z

@pyokagan Sorry for the inconvenience caused. This should due to my mistake during rebasing.

CanIHasReview-bot · 2019-05-03T07:40:42Z

v12

@fzdy1914 submitted v12 for review.

[v12 1/7] build.gradle: fix checkstyle plugin failure
[v12 2/7] build.gradle: add platform-specific javafx runtime dependencies
[v12 3/7] Change the entry point of addressbook
[v12 4/7] build.gradle: add javafx runtime dependency for all platforms
[v12 5/7] Workaround headless test failure on Windows OS
[v12 6/7] Workaround coveralls upload failure on Travis CI
[v12 7/7] Upgrade to JDK 11

(📚 Archive) (📈 Interdiff between v11 and v12) (📈 Range-Diff between v11 and v12)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/961/12/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan · 2019-05-03T08:23:26Z

Bah, looks like BrowserPanelTest is flaky. I did see it while reviewing #959 , but I thought it was due to some bugs in previous iterations of the PR (I didn't get it on the final iteration that was merged). Looks like it is due to some other problem though.

pyokagan · 2019-05-03T08:44:22Z

Maybe it's unrelated but I noted that:

addressbook-level4/src/test/java/guitests/guihandles/BrowserPanelHandle.java

Lines 28 to 32 in 10586cb

    
           if (newState == Worker.State.RUNNING) { 
        
               isWebViewLoaded = false; 
        
           } else if (newState == Worker.State.SUCCEEDED) { 
        
               isWebViewLoaded = true; 
        
           }

combined with

addressbook-level4/src/test/java/guitests/guihandles/BrowserPanelHandle.java

Lines 61 to 63 in 10586cb

    
           public boolean isLoaded() { 
        
               return isWebViewLoaded; 
        
           }

may be thread unsafe because they are being written and read from different threads, and I'm not sure if Java provides any guarantees on the atomicity of normal field writing.

pyokagan · 2019-05-03T08:48:48Z

Anyway, for correctness perhaps the isWebViewLoaded field should be declared volatile to ensure that Java immediately stores any changes to memory and does not keep it around in registers or thread-local memory or something.

fzdy1914 force-pushed the rebased branch 2 times, most recently from f428dff to d3cd6e6 Compare February 8, 2019 13:57

fzdy1914 force-pushed the rebased branch 4 times, most recently from b7b4d39 to bae532a Compare February 20, 2019 04:17

CanIHasReview-bot added the s.ToReview label Feb 20, 2019

pyokagan added s.Ongoing and removed s.ToReview labels Feb 22, 2019

fzdy1914 force-pushed the rebased branch from 203a5d3 to 5c978af Compare February 28, 2019 14:13

fzdy1914 force-pushed the rebased branch from 5c978af to 7dc9331 Compare February 28, 2019 14:46

fzdy1914 force-pushed the rebased branch 2 times, most recently from 7a190cb to 41b0bb5 Compare March 5, 2019 06:48

fzdy1914 force-pushed the rebased branch from 41b0bb5 to 8c0c986 Compare March 12, 2019 16:10

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 13, 2019

pyokagan reviewed Mar 14, 2019

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

pyokagan reviewed Mar 14, 2019

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

build.gradle Show resolved Hide resolved

fzdy1914 force-pushed the rebased branch from cf33a6b to d50e69e Compare May 3, 2019 03:14

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels May 3, 2019

CanIHasReview-bot requested a review from pyokagan May 3, 2019 03:18

pyokagan suggested changes May 3, 2019

View reviewed changes

pyokagan added s.Ongoing and removed s.ToReview labels May 3, 2019

fzdy1914 and others added 4 commits May 3, 2019 15:35

fzdy1914 force-pushed the rebased branch from d50e69e to 0364a89 Compare May 3, 2019 07:38

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels May 3, 2019

CanIHasReview-bot requested a review from pyokagan May 3, 2019 07:40

pyokagan approved these changes May 3, 2019

View reviewed changes

pyokagan changed the title ~~Support JDK11+~~ Migrate to JDK11+ May 3, 2019

pyokagan changed the title ~~Migrate to JDK11+~~ Migrate to JDK 11+ May 3, 2019

pyokagan merged commit 10586cb into se-edu:master May 3, 2019

pyokagan added s.ToMerge and removed s.ToReview labels May 3, 2019

fzdy1914 deleted the rebased branch May 3, 2019 08:43

pyokagan mentioned this pull request May 3, 2019

Distributing AddressBook-Level4 in a JDK11+ world #951

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to JDK 11+ #961

Migrate to JDK 11+ #961

fzdy1914 commented Feb 8, 2019

CanIHasReview-bot commented Feb 8, 2019

fzdy1914 commented Feb 20, 2019

pyokagan commented Feb 20, 2019

CanIHasReview-bot commented Feb 20, 2019

pyokagan commented Feb 22, 2019

pyokagan commented Feb 22, 2019

pyokagan commented Feb 22, 2019

fzdy1914 commented Feb 28, 2019

pyokagan commented Feb 28, 2019

fzdy1914 commented Feb 28, 2019

fzdy1914 commented Feb 28, 2019

CanIHasReview-bot commented Mar 13, 2019

pyokagan left a comment

pyokagan left a comment

CanIHasReview-bot commented May 3, 2019

pyokagan left a comment

fzdy1914 commented May 3, 2019

CanIHasReview-bot commented May 3, 2019

pyokagan commented May 3, 2019

pyokagan commented May 3, 2019

pyokagan commented May 3, 2019

Migrate to JDK 11+ #961

Migrate to JDK 11+ #961

Conversation

fzdy1914 commented Feb 8, 2019

CanIHasReview-bot commented Feb 8, 2019

fzdy1914 commented Feb 20, 2019

pyokagan commented Feb 20, 2019

CanIHasReview-bot commented Feb 20, 2019

v1

pyokagan commented Feb 22, 2019

pyokagan commented Feb 22, 2019

pyokagan commented Feb 22, 2019

fzdy1914 commented Feb 28, 2019

pyokagan commented Feb 28, 2019

fzdy1914 commented Feb 28, 2019

fzdy1914 commented Feb 28, 2019

CanIHasReview-bot commented Mar 13, 2019

v2

pyokagan left a comment

Choose a reason for hiding this comment

pyokagan left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented May 3, 2019

v11

pyokagan left a comment

Choose a reason for hiding this comment

fzdy1914 commented May 3, 2019

CanIHasReview-bot commented May 3, 2019

v12

pyokagan commented May 3, 2019

pyokagan commented May 3, 2019

pyokagan commented May 3, 2019