You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/04/22 22:21:31 UTC

[GitHub] [beam] Hannah-Jiang opened a new pull request #11502: [BEAM-9797] use tox for license pulling

Hannah-Jiang opened a new pull request #11502:
URL: https://github.com/apache/beam/pull/11502


   **Please** add a meaningful description for your change here
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   Post-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
   --- | --- | --- | --- | --- | --- | --- | ---
   Go | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
   Java | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
   Python | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
   XLang | --- | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
   
   Pre-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   --- |Java | Python | Go | Website
   --- | --- | --- | --- | ---
   Non-portable | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) 
   Portable | --- | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
   
   See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r414930577



##########
File path: sdks/java/container/license_scripts/license_script.sh
##########
@@ -15,39 +15,33 @@
  # limitations under the License.
 
 set -e
+ROOT_DIR=$(pwd)
+CONTAINER_DIR="$ROOT_DIR/sdks/java/container"
+
 # reports are generated at ~/beam/java_third_party_licenses
 ./gradlew generateLicenseReport --rerun-tasks
 
-# install packages needed for pull_licenses_java.py
-pip install "beautifulsoup4>=4.9.0,<5.0"
-pip install "future>=0.16.0,<1.0.0"
-pip install "pyyaml>=3.12,<6.0.0"
-pip install "tenacity>=5.0.2,<6.0"
+# pip install 'tox>=3.14.6,<3.15.0'
+pip install --no-cache-dir tox==3.11.1

Review comment:
       ok, `virtualenv` worked.
   Please take a look the PR when you have time.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] udim commented on a change in pull request #11502: [BEAM-9797] use virtualenv for Java license pulling

Posted by GitBox <gi...@apache.org>.
udim commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r418178166



##########
File path: sdks/java/container/license_scripts/pull_licenses_java.py
##########
@@ -35,18 +35,9 @@
 from tenacity import retry
 from tenacity import stop_after_attempt
 from tenacity import wait_exponential
+from urllib.request import urlopen, URLError, HTTPError
+
 
-try:
-    # py2
-    from future.moves.urllib.request import urlopen
-    from future.moves.urllib.request import URLError, HTTPError
-except:
-    # py3
-    from future import standard_library
-    from urllib.request import urlopen, URLError, HTTPError

Review comment:
       I think your setup is fine - it defaults to Python 2. Since the `from urllib.request import urlopen, URLError, HTTPErro` line doesn't work on 2.x, adding the flag should be a universal solution.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] udim commented on a change in pull request #11502: [BEAM-9797] use virtualenv for Java license pulling

Posted by GitBox <gi...@apache.org>.
udim commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r417704147



##########
File path: sdks/java/container/license_scripts/pull_licenses_java.py
##########
@@ -35,18 +35,9 @@
 from tenacity import retry
 from tenacity import stop_after_attempt
 from tenacity import wait_exponential
+from urllib.request import urlopen, URLError, HTTPError
+
 
-try:
-    # py2
-    from future.moves.urllib.request import urlopen
-    from future.moves.urllib.request import URLError, HTTPError
-except:
-    # py3
-    from future import standard_library
-    from urllib.request import urlopen, URLError, HTTPError

Review comment:
       Does  adding `--python=python3` to the virtualenv command fix this for you, Brian?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-618714560


   Run Java PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-619266312


   Run Java PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-618721532


   It works fine locally, however, fails with Jenkins. The reason is tox is not installed successfully. I tried to match tox version used by Py Precommit test, but still no luck. It seems like Python use virtualenv to install tox, while with this PR, tox is installed Jenkins itself.
   Error log is here: https://scans.gradle.com/s/bcwhc4gwfqnes/console-log?task=:sdks:java:container:generateThirdPartyLicenses
   
   @udim, do you have any ideas how to fix it?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] TheNeuralBit commented on a change in pull request #11502: [BEAM-9797] use virtualenv for Java license pulling

Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r418163527



##########
File path: sdks/java/container/license_scripts/pull_licenses_java.py
##########
@@ -35,18 +35,9 @@
 from tenacity import retry
 from tenacity import stop_after_attempt
 from tenacity import wait_exponential
+from urllib.request import urlopen, URLError, HTTPError
+
 
-try:
-    # py2
-    from future.moves.urllib.request import urlopen
-    from future.moves.urllib.request import URLError, HTTPError
-except:
-    # py3
-    from future import standard_library
-    from urllib.request import urlopen, URLError, HTTPError

Review comment:
       > Can you please check what version of Python was installed at sdks/java/container/build/virtualenv?
   ```
   ❯ ./sdks/java/container/build/virtualenv/bin/python --version
   Python 2.7.18rc1
   ```
   
   If this is working for others maybe I just have something else wrong with my setup...
   
   > Does adding --python=python3 to the virtualenv command fix this for you, Brian?
   yeah that's what I did to get it passing. I thought about putting up a PR for the change, but not sure if that's a universal solution or not.

##########
File path: sdks/java/container/license_scripts/pull_licenses_java.py
##########
@@ -35,18 +35,9 @@
 from tenacity import retry
 from tenacity import stop_after_attempt
 from tenacity import wait_exponential
+from urllib.request import urlopen, URLError, HTTPError
+
 
-try:
-    # py2
-    from future.moves.urllib.request import urlopen
-    from future.moves.urllib.request import URLError, HTTPError
-except:
-    # py3
-    from future import standard_library
-    from urllib.request import urlopen, URLError, HTTPError

Review comment:
       > Can you please check what version of Python was installed at sdks/java/container/build/virtualenv?
   ```
   ❯ ./sdks/java/container/build/virtualenv/bin/python --version
   Python 2.7.18rc1
   ```
   
   If this is working for others maybe I just have something else wrong with my setup...
   
   > Does adding --python=python3 to the virtualenv command fix this for you, Brian?
   
   yeah that's what I did to get it passing. I thought about putting up a PR for the change, but not sure if that's a universal solution or not.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r414209813



##########
File path: sdks/java/container/license_scripts/license_script.sh
##########
@@ -15,39 +15,33 @@
  # limitations under the License.
 
 set -e
+ROOT_DIR=$(pwd)
+CONTAINER_DIR="$ROOT_DIR/sdks/java/container"
+
 # reports are generated at ~/beam/java_third_party_licenses
 ./gradlew generateLicenseReport --rerun-tasks
 
-# install packages needed for pull_licenses_java.py
-pip install "beautifulsoup4>=4.9.0,<5.0"
-pip install "future>=0.16.0,<1.0.0"
-pip install "pyyaml>=3.12,<6.0.0"
-pip install "tenacity>=5.0.2,<6.0"
+# pip install 'tox>=3.14.6,<3.15.0'
+pip install --no-cache-dir tox==3.11.1

Review comment:
       and probably, using tox or virtualenv installs more packages then original approaches.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on issue #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on issue #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-618071495


   R: @udim 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang removed a comment on pull request #11502: [BEAM-9797] use virtualenv for Java license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang removed a comment on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-618768852






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r414216957



##########
File path: sdks/java/container/license_scripts/license_script.sh
##########
@@ -15,39 +15,33 @@
  # limitations under the License.
 
 set -e
+ROOT_DIR=$(pwd)
+CONTAINER_DIR="$ROOT_DIR/sdks/java/container"
+
 # reports are generated at ~/beam/java_third_party_licenses
 ./gradlew generateLicenseReport --rerun-tasks
 
-# install packages needed for pull_licenses_java.py
-pip install "beautifulsoup4>=4.9.0,<5.0"
-pip install "future>=0.16.0,<1.0.0"
-pip install "pyyaml>=3.12,<6.0.0"
-pip install "tenacity>=5.0.2,<6.0"
+# pip install 'tox>=3.14.6,<3.15.0'
+pip install --no-cache-dir tox==3.11.1

Review comment:
       ok, I will try with virtualenv.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang edited a comment on pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang edited a comment on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-618721532


   It works fine locally, however, fails with Jenkins. The reason is tox is not installed successfully. I tried to match tox version used by Py Precommit test, but still no luck. It seems like Python use virtualenv to install tox, while with this PR, tox is installed in Jenkins itself.
   Error log is here: https://scans.gradle.com/s/bcwhc4gwfqnes/console-log?task=:sdks:java:container:generateThirdPartyLicenses
   
   @udim, do you have any ideas how to fix it?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] udim commented on pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
udim commented on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-618726574


   > It works fine locally, however, fails with Jenkins. The reason is tox is not installed successfully. I tried to match tox version used by Py Precommit test, but still no luck. It seems like Python use virtualenv to install tox, while with this PR, tox is installed in Jenkins itself.
   > Error log is here: https://scans.gradle.com/s/bcwhc4gwfqnes/console-log?task=:sdks:java:container:generateThirdPartyLicenses
   > 
   > @udim, do you have any ideas how to fix it?
   
   I don't know if this will work, but could you try adding 
   ```
   applyPythonNature()
   pythonVersion = '3.7'
   // might need this too, not sure:
   // apply from: "../common.gradle"
   ```
   to the build.gradle file and use toxTask to define the task?
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on pull request #11502: [BEAM-9797] use virtualenv for Java license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-620221255


   A kind reminder, @udim 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-618768852


   Run Java PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-619783844


   Run Java PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] udim commented on a change in pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
udim commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r414207242



##########
File path: sdks/java/container/license_scripts/license_script.sh
##########
@@ -15,39 +15,33 @@
  # limitations under the License.
 
 set -e
+ROOT_DIR=$(pwd)
+CONTAINER_DIR="$ROOT_DIR/sdks/java/container"
+
 # reports are generated at ~/beam/java_third_party_licenses
 ./gradlew generateLicenseReport --rerun-tasks
 
-# install packages needed for pull_licenses_java.py
-pip install "beautifulsoup4>=4.9.0,<5.0"
-pip install "future>=0.16.0,<1.0.0"
-pip install "pyyaml>=3.12,<6.0.0"
-pip install "tenacity>=5.0.2,<6.0"
+# pip install 'tox>=3.14.6,<3.15.0'
+pip install --no-cache-dir tox==3.11.1

Review comment:
       The solution could be to do something like:
   ```sh
   # Creates a new directory $env_path and installs a virtual environment in it.
   env_path=$ROOT_DIR/build/license_scripts_env
   virtualenv $env_path --python python3.7
   source $env_path/bin/activate
   # Install tox, or just install the packages directly. Might be less complicated to skip using tox in this case.
   pip install ...
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] udim commented on a change in pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
udim commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r414756022



##########
File path: sdks/java/container/license_scripts/license_script.sh
##########
@@ -15,39 +15,33 @@
  # limitations under the License.
 
 set -e
+ROOT_DIR=$(pwd)
+CONTAINER_DIR="$ROOT_DIR/sdks/java/container"
+
 # reports are generated at ~/beam/java_third_party_licenses
 ./gradlew generateLicenseReport --rerun-tasks
 
-# install packages needed for pull_licenses_java.py
-pip install "beautifulsoup4>=4.9.0,<5.0"
-pip install "future>=0.16.0,<1.0.0"
-pip install "pyyaml>=3.12,<6.0.0"
-pip install "tenacity>=5.0.2,<6.0"
+# pip install 'tox>=3.14.6,<3.15.0'
+pip install --no-cache-dir tox==3.11.1

Review comment:
       My fault, I didn't know python3 venv wasn't properly installed on our Jenkins nodes.
   Our gradle scripts for Python tests use the `virtualenv` command.
   
   Jenkins jobs should not be installing system-wide packages.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] udim commented on a change in pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
udim commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r414212691



##########
File path: sdks/java/container/license_scripts/license_script.sh
##########
@@ -15,39 +15,33 @@
  # limitations under the License.
 
 set -e
+ROOT_DIR=$(pwd)
+CONTAINER_DIR="$ROOT_DIR/sdks/java/container"
+
 # reports are generated at ~/beam/java_third_party_licenses
 ./gradlew generateLicenseReport --rerun-tasks
 
-# install packages needed for pull_licenses_java.py
-pip install "beautifulsoup4>=4.9.0,<5.0"
-pip install "future>=0.16.0,<1.0.0"
-pip install "pyyaml>=3.12,<6.0.0"
-pip install "tenacity>=5.0.2,<6.0"
+# pip install 'tox>=3.14.6,<3.15.0'
+pip install --no-cache-dir tox==3.11.1

Review comment:
       The virtualenv command should already be installed, and all the changes it makes are limited to the given directory.
   Once you run the `activate` command `pip install` will no longer change the user environment.
   
   Actually, according to Beam's guide, Python 3 already has a built-in virtualenv tool:
   ```
   python3 -m venv env && source env/bin/activate
   ```
   https://beam.apache.org/documentation/sdks/python-dependencies/




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-618670993


   Run Java PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] TheNeuralBit commented on a change in pull request #11502: [BEAM-9797] use virtualenv for Java license pulling

Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r417686263



##########
File path: sdks/java/container/license_scripts/pull_licenses_java.py
##########
@@ -35,18 +35,9 @@
 from tenacity import retry
 from tenacity import stop_after_attempt
 from tenacity import wait_exponential
+from urllib.request import urlopen, URLError, HTTPError
+
 
-try:
-    # py2
-    from future.moves.urllib.request import urlopen
-    from future.moves.urllib.request import URLError, HTTPError
-except:
-    # py3
-    from future import standard_library
-    from urllib.request import urlopen, URLError, HTTPError

Review comment:
       @Hannah-Jiang @udim should we keep the py2 imports for now? I still have python 2 as the default `python` on my machine, so this change broke :sdks:java:container:docker for me
   
   Or maybe I should have changed the default by now?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11502: [BEAM-9797] use virtualenv for Java license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r417696355



##########
File path: sdks/java/container/license_scripts/pull_licenses_java.py
##########
@@ -35,18 +35,9 @@
 from tenacity import retry
 from tenacity import stop_after_attempt
 from tenacity import wait_exponential
+from urllib.request import urlopen, URLError, HTTPError
+
 
-try:
-    # py2
-    from future.moves.urllib.request import urlopen
-    from future.moves.urllib.request import URLError, HTTPError
-except:
-    # py3
-    from future import standard_library
-    from urllib.request import urlopen, URLError, HTTPError

Review comment:
       This is running in a virtualenv as implemented [here](https://github.com/apache/beam/blob/master/sdks/java/container/license_scripts/license_script.sh#L28).
   
   When I check locally with py2, it created py3.7 env. Default Python version on Jenkins is also 2.7 and the script run successfully. Maybe in both cases, Python3 was installed on the machine, so it is able to create virtualenv with Python3?
   
   Can you please check what version of Python was installed at sdks/java/container/build/virtualenv?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] TheNeuralBit commented on a change in pull request #11502: [BEAM-9797] use virtualenv for Java license pulling

Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r418181589



##########
File path: sdks/java/container/license_scripts/pull_licenses_java.py
##########
@@ -35,18 +35,9 @@
 from tenacity import retry
 from tenacity import stop_after_attempt
 from tenacity import wait_exponential
+from urllib.request import urlopen, URLError, HTTPError
+
 
-try:
-    # py2
-    from future.moves.urllib.request import urlopen
-    from future.moves.urllib.request import URLError, HTTPError
-except:
-    # py3
-    from future import standard_library
-    from urllib.request import urlopen, URLError, HTTPError

Review comment:
       Well what I'm wondering is if `--python=python3` always works, or if it only works if you have a `python3` symlink, etc...




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r414209813



##########
File path: sdks/java/container/license_scripts/license_script.sh
##########
@@ -15,39 +15,33 @@
  # limitations under the License.
 
 set -e
+ROOT_DIR=$(pwd)
+CONTAINER_DIR="$ROOT_DIR/sdks/java/container"
+
 # reports are generated at ~/beam/java_third_party_licenses
 ./gradlew generateLicenseReport --rerun-tasks
 
-# install packages needed for pull_licenses_java.py
-pip install "beautifulsoup4>=4.9.0,<5.0"
-pip install "future>=0.16.0,<1.0.0"
-pip install "pyyaml>=3.12,<6.0.0"
-pip install "tenacity>=5.0.2,<6.0"
+# pip install 'tox>=3.14.6,<3.15.0'
+pip install --no-cache-dir tox==3.11.1

Review comment:
       and probably, using tox or virtualenv installs more packages then the original approach.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r414208940



##########
File path: sdks/java/container/license_scripts/license_script.sh
##########
@@ -15,39 +15,33 @@
  # limitations under the License.
 
 set -e
+ROOT_DIR=$(pwd)
+CONTAINER_DIR="$ROOT_DIR/sdks/java/container"
+
 # reports are generated at ~/beam/java_third_party_licenses
 ./gradlew generateLicenseReport --rerun-tasks
 
-# install packages needed for pull_licenses_java.py
-pip install "beautifulsoup4>=4.9.0,<5.0"
-pip install "future>=0.16.0,<1.0.0"
-pip install "pyyaml>=3.12,<6.0.0"
-pip install "tenacity>=5.0.2,<6.0"
+# pip install 'tox>=3.14.6,<3.15.0'
+pip install --no-cache-dir tox==3.11.1

Review comment:
       This needs to install virtualenv and changes user environment as well. How about keep the original approach (installing pyyaml, tenacity etc) but remove the uninstall part to avoid race conditions?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r414277855



##########
File path: sdks/java/container/license_scripts/license_script.sh
##########
@@ -15,39 +15,33 @@
  # limitations under the License.
 
 set -e
+ROOT_DIR=$(pwd)
+CONTAINER_DIR="$ROOT_DIR/sdks/java/container"
+
 # reports are generated at ~/beam/java_third_party_licenses
 ./gradlew generateLicenseReport --rerun-tasks
 
-# install packages needed for pull_licenses_java.py
-pip install "beautifulsoup4>=4.9.0,<5.0"
-pip install "future>=0.16.0,<1.0.0"
-pip install "pyyaml>=3.12,<6.0.0"
-pip install "tenacity>=5.0.2,<6.0"
+# pip install 'tox>=3.14.6,<3.15.0'
+pip install --no-cache-dir tox==3.11.1

Review comment:
       I tried with virtureenv. It works fine locally, but fails with Jenkins. 
   
   **Try1:**
   ```
   python3 -m venv env && source env/bin/activate
   ```
   Error: 
   ```
   21:01:17 The virtual environment was not created successfully because ensurepip is not
   21:01:17 available.  On Debian/Ubuntu systems, you need to install the python3-venv
   21:01:17 package using the following command.
   21:01:17 
   21:01:17     apt-get install python3-venv
   21:01:17 
   21:01:17 You may need to use sudo with that command.  After installing the python3-venv
   21:01:17 package, recreate your virtual environment.
   21:01:17 
   21:01:17 Failing command: ['/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_Commit/src/env/bin/python3', '-Im', 'ensurepip', '--upgrade', '--default-pip']
   ```
   It failed to create virtueenv and ran with default local python env, which is py2.7.
    
   **Try2:**
   ```
   python3 -m venv env && source env/bin/activate || pip install virtualenv && virtualenv env && source env/bin/activate
   ```
   Ok, this time, if virtueenv with py3 fails, create it with py2. Both commands failed and ran at default py env.
   Error: [link](https://builds.apache.org/job/beam_PreCommit_Java_Commit/11004/consoleFull)
   ```
   20:24:40 Using base prefix '/usr'
   20:24:40 New python executable in /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_Commit/src/env/bin/python3
   20:24:41 Traceback (most recent call last):
   20:24:41   File "/usr/local/bin/virtualenv", line 11, in <module>
   20:24:41     sys.exit(main())
   20:24:41   File "/usr/local/lib/python3.5/dist-packages/virtualenv.py", line 793, in main
   20:24:41     symlink=options.symlink,
   20:24:41   File "/usr/local/lib/python3.5/dist-packages/virtualenv.py", line 1071, in create_environment
   20:24:41     install_python(home_dir, lib_dir, inc_dir, bin_dir, site_packages=site_packages, clear=clear, symlink=symlink)
   20:24:41   File "/usr/local/lib/python3.5/dist-packages/virtualenv.py", line 1457, in install_python
   20:24:41     shutil.copyfile(executable, py_executable)
   20:24:41   File "/usr/lib/python3.5/shutil.py", line 98, in copyfile
   20:24:41     raise SameFileError("{!r} and {!r} are the same file".format(src, dst))
   20:24:41 shutil.SameFileError: '/usr/bin/python3' and '/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_Commit/src/env/bin/python3' are the same file
   ```
   **Try3:**
   ```
   apt-get install python3-venv
   python3 -m venv env && source env/bin/activate
   ```
   In order to apt-get install, need to run with sudo.
   
   **Try4:**
   ```
   sudo apt-get install python3-venv
   python3 -m venv env && source env/bin/activate
   ```
   Added sudo, and it asks to input password and failed.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] udim commented on a change in pull request #11502: [BEAM-9797] use virtualenv for Java license pulling

Posted by GitBox <gi...@apache.org>.
udim commented on a change in pull request #11502:
URL: https://github.com/apache/beam/pull/11502#discussion_r418187314



##########
File path: sdks/java/container/license_scripts/pull_licenses_java.py
##########
@@ -35,18 +35,9 @@
 from tenacity import retry
 from tenacity import stop_after_attempt
 from tenacity import wait_exponential
+from urllib.request import urlopen, URLError, HTTPError
+
 
-try:
-    # py2
-    from future.moves.urllib.request import urlopen
-    from future.moves.urllib.request import URLError, HTTPError
-except:
-    # py3
-    from future import standard_library
-    from urllib.request import urlopen, URLError, HTTPError

Review comment:
       Yes, we use it elsewhere in our build (BeamModulePlugin.groovy)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] udim commented on pull request #11502: [BEAM-9797] use tox for license pulling

Posted by GitBox <gi...@apache.org>.
udim commented on pull request #11502:
URL: https://github.com/apache/beam/pull/11502#issuecomment-618726999


   Oh I just realized it's a script, not a build.gradle file


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org