CPDBear: Fix for crash when many files are given #1429

impmihai · 2017-02-13T23:58:58Z

Until now, the files to run on were given to CPD with
the --files option, which would raise
'OSError: [Errno 7] Argument list too long'. Now, the
files are saved in a temporary file, and that file is
given to CPD with the --filelist option, which takes a
csv file containing a list of files to run on.

Fixes coala/coala#3701

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

I read the commit guidelines and I've followed
them.
I ran coala over my code locally. (All commits have to pass
individually. It is not sufficient to have "fixup commits" on your PR,
our bot will still report the issues for the previous commit.) You will
likely receive a lot of bot comments and build failures if coala does not
pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

pick up any PR at https://coala.io/review
review it (check https://coala.io/reviewing for more info)
if you are sure that it needs work, use cobot mark wip <URL> to get it out
of the review queue.

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

adtac · 2017-02-15T11:24:55Z

circle.yml

@@ -11,7 +11,7 @@ dependencies:
    - ~/.cabal
    - ~/infer-linux64-v0.7.0
    - ~/nltk_data
-    - ~/pmd-bin-5.4.1
+    - ~/pmd-bin-5.5.3


You forogt to change the version in .travis.yml ;)

oh, thanks! I really missed that out

Makman2 · 2017-02-15T16:06:51Z

bears/general/CPDBear.py

@@ -102,6 +108,8 @@ def run(self, language: str, minimum_tokens: int=20,

        stdout_output, _ = run_shell_command(arguments)

+        os.unlink(temp_filepath)


maybe use os.remove? Seems they are functionally identical...

Imo os.remove is better to understand, what do you think? :)

Makman2 · 2017-02-15T16:10:53Z

bears/general/CPDBear.py

@@ -102,6 +108,8 @@ def run(self, language: str, minimum_tokens: int=20,

        stdout_output, _ = run_shell_command(arguments)

+        os.unlink(temp_filepath)


also here, why not use temp.name? :)

I was not sure that the object still had the file name after closing, I will change that

As delete=False I think it should be still valid 👍

Alternatively, you could use with ... as ... :) (Not completely sure if it exists though)

impmihai · 2017-02-15T22:12:19Z

I see that on JavaPMDBear, the root directory is sent to pmd do analyze. Should i do that here too? But this would void the ignored files

Makman2 · 2017-02-15T22:30:16Z

@impmihai does this improve the results? Or is it just a shortcut for CPD to grab all files it can find?
If it's 2nd, then you are right, we don't want to void ignored files ;)

impmihai · 2017-02-16T13:32:40Z

@Makman2 it's the second. Then I am waiting for the dependency fixes to be pushed before finishing this up. Thanks for the review! :)

Makman2 · 2017-02-16T16:46:20Z

Do you need dependency fixes? Just don't pass the directory :O

impmihai · 2017-06-01T19:21:44Z

May I get some help with the tests? It is failing tests from other bears, I have not yet written a test for it because I wanted to have a code that works firstly. But this is really hard for me to understand why i get coverage issues like it throws,..

Makman2 · 2017-06-01T22:45:07Z

Those are weird indeed... try to rebase, if fails reoccur we need to investigate more^^

Makman2 · 2017-06-01T22:45:45Z

Or if you are already up-to-date:

git commit --amend --no-edit && git push -f

to regenerate the last commit hash.

gitmate-bot · 2017-06-09T00:37:10Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-06-15T23:17:07Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-06-22T21:57:31Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-06-29T20:37:27Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

AsnelChristian · 2017-06-30T18:38:25Z

your tests are still failing :/

AsnelChristian · 2017-06-30T18:50:51Z

.ci/deps.java.sh

-if [ ! -e ~/pmd-bin-5.4.1/bin ]; then
-  wget -nc -O ~/pmd.zip https://github.com/pmd/pmd/releases/download/pmd_releases%2F5.4.1/pmd-bin-5.4.1.zip
+if [ ! -e ~/pmd-bin-5.6.1/bin ]; then
+  wget -nc -O ~/pmd.zip https://github.com/pmd/pmd/releases/download/pmd_releases%2F5.6.1/pmd-bin-5.6.1.zip


you should update pmd in a different commit

jayvdb · 2018-07-20T08:37:49Z

.ci/deps.java.sh

-if [ ! -e ~/pmd-bin-5.4.1/bin ]; then
-  wget -nc -O ~/pmd.zip https://github.com/pmd/pmd/releases/download/pmd_releases%2F5.4.1/pmd-bin-5.4.1.zip
+if [ ! -e ~/pmd-bin-5.6.1/bin ]; then
+  wget -nc -O ~/pmd.zip https://github.com/pmd/pmd/releases/download/pmd_releases%2F5.6.1/pmd-bin-5.6.1.zip


Upgrading the bear dependency is an intrinsic part of this issue, so IMO it can stay in this commit.

gitmate-bot · 2017-07-07T17:30:29Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-07-14T16:10:38Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-07-22T01:57:23Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Until now, the files to run on were given to CPD with the --files option, which would raise 'OSError: [Errno 7] Argument list too long'. Now, the files are saved in a temporary file, and that file is given to CPD with the --filelist option, which takes a csv file containing a list of files to run on. Fixes coala/coala#3701

jayvdb · 2018-04-19T06:24:45Z

bears/general/CPDBear.py

@@ -106,6 +114,8 @@ def run(self, language: language,

        stdout_output, _ = run_shell_command(arguments)

+        os.remove(tmp_filename)
+
        if stdout_output:


please change this to

if not stdout_output: return ...

and if you dont want to write a test, ...

if not stdout_output: # pragma: no cover return ...

Oh, ignore me, this is unrelated to the patch.

Create and use variables for the versions, so that the PATH line does not need to be modified so often, and the merge conflicts that causes. Use ~/.local/ where possible for installed software, so that scripts can be reused. Related to coala#1429 Related to coala#2638

jayvdb · 2018-07-20T08:37:49Z

.ci/deps.java.sh

-if [ ! -e ~/pmd-bin-5.4.1/bin ]; then
-  wget -nc -O ~/pmd.zip https://github.com/pmd/pmd/releases/download/pmd_releases%2F5.4.1/pmd-bin-5.4.1.zip
+if [ ! -e ~/pmd-bin-5.6.1/bin ]; then
+  wget -nc -O ~/pmd.zip https://github.com/pmd/pmd/releases/download/pmd_releases%2F5.6.1/pmd-bin-5.6.1.zip


Upgrading the bear dependency is an intrinsic part of this issue, so IMO it can stay in this commit.

jayvdb · 2018-07-20T08:41:23Z

bears/general/CPDBear.py

@@ -106,6 +114,8 @@ def run(self, language: language,

        stdout_output, _ = run_shell_command(arguments)

+        os.remove(tmp_filename)
+
        if stdout_output:


Oh, ignore me, this is unrelated to the patch.

jayvdb · 2018-07-26T04:35:59Z

.travis.yml

@@ -87,7 +87,7 @@ env:
    - USE_PPAS="marutter/rdev staticfloat/juliareleases ondrej/golang"
    - R_LIB_USER=~/R/Library
    - LINTR_COMMENT_BOT=false
-    - PATH="/opt/cabal/1.24/bin:$PATH:$TRAVIS_BUILD_DIR/node_modules/.bin:$TRAVIS_BUILD_DIR/vendor/bin:$HOME/dart-sdk/bin:$HOME/.cabal/bin:$HOME/infer-linux64-v0.7.0/infer/bin:$HOME/pmd-bin-5.4.1/bin:$HOME/bakalint-0.4.0:$HOME/elm-format-0.18:$HOME/.local/tailor/tailor-latest/bin:$HOME/phpmd"
+    - PATH="/opt/cabal/1.24/bin:$PATH:$TRAVIS_BUILD_DIR/node_modules/.bin:$TRAVIS_BUILD_DIR/vendor/bin:$HOME/dart-sdk/bin:$HOME/.cabal/bin:$HOME/infer-linux64-v0.7.0/infer/bin:$HOME/pmd-bin-5.6.1/bin:$HOME/bakalint-0.4.0:$HOME/elm-format-0.18:$HOME/.local/tailor/tailor-latest/bin:$HOME/phpmd"


This change wont be required any more

jayvdb · 2018-07-26T04:36:40Z

bears/general/CPDBear.py

@@ -102,6 +109,8 @@ def run(self, language: str, minimum_tokens: int=20,

        stdout_output, _ = run_shell_command(arguments)

+        os.remove(tmp_filename)


While useful, I dont think it is worth the effort.

jayvdb · 2018-07-26T04:37:47Z

circle.yml

@@ -13,7 +13,7 @@ dependencies:
    - ~/.ghc-mod
    - ~/infer-linux64-v0.7.0
    - ~/nltk_data
-    - ~/pmd-bin-5.4.1
+    - ~/pmd-bin-5.6.1


The changes to circle.yml shouldnt be needed any more due to a724ad7

gitmate-bot added process/pending review size/XS labels Feb 13, 2017

impmihai force-pushed the fix-cpd-many-files branch 4 times, most recently from 7f082b7 to 8f2ec83 Compare February 14, 2017 02:51

gitmate-bot added size/S review carefully! and removed size/XS labels Feb 14, 2017

impmihai force-pushed the fix-cpd-many-files branch 2 times, most recently from 84a11db to c262854 Compare February 15, 2017 01:47

adtac reviewed Feb 15, 2017

View reviewed changes

adtac added process/wip and removed process/pending review labels Feb 15, 2017

Makman2 reviewed Feb 15, 2017

View reviewed changes

impmihai force-pushed the fix-cpd-many-files branch from c262854 to 5f8d104 Compare February 20, 2017 10:23

gitmate-bot added process/pending review and removed process/wip labels Feb 20, 2017

impmihai force-pushed the fix-cpd-many-files branch 3 times, most recently from 055b403 to 4920f87 Compare February 21, 2017 22:09

jayvdb added process/wip and removed process/pending review labels Feb 26, 2017

impmihai force-pushed the fix-cpd-many-files branch from 4920f87 to 51ee989 Compare February 27, 2017 08:31

gitmate-bot removed the process/wip label Feb 27, 2017

AsnelChristian requested changes Jun 30, 2017

View reviewed changes

co-robo added process/wip and removed process/pending review labels Jun 30, 2017

gitmate-bot added the status/STALE label Aug 31, 2017

jayvdb force-pushed the fix-cpd-many-files branch from 877c242 to 4e1de2b Compare April 19, 2018 04:15

gitmate-bot added process/pending review size/XS type/bug area/bears and removed process/wip size/M labels Apr 19, 2018

gitmate-bot assigned impmihai Apr 19, 2018

jayvdb requested changes Apr 19, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels Apr 19, 2018

jayvdb mentioned this pull request Jul 23, 2018

.travis.yml: Remove version numbers from PATH line #2631

Merged

2 tasks

jayvdb requested changes Jul 26, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPDBear: Fix for crash when many files are given #1429

CPDBear: Fix for crash when many files are given #1429

impmihai commented Feb 13, 2017

adtac Feb 15, 2017

impmihai Feb 15, 2017

Makman2 Feb 15, 2017

Makman2 Feb 15, 2017

impmihai Feb 15, 2017

Makman2 Feb 15, 2017

adtac Feb 16, 2017 •

edited

Loading

impmihai commented Feb 15, 2017

Makman2 commented Feb 15, 2017 •

edited

Loading

impmihai commented Feb 16, 2017

Makman2 commented Feb 16, 2017

impmihai commented Jun 1, 2017

Makman2 commented Jun 1, 2017

Makman2 commented Jun 1, 2017

gitmate-bot commented Jun 9, 2017

gitmate-bot commented Jun 15, 2017

gitmate-bot commented Jun 22, 2017

gitmate-bot commented Jun 29, 2017

AsnelChristian commented Jun 30, 2017

AsnelChristian Jun 30, 2017

This comment was marked as outdated.

jayvdb Jul 20, 2018

gitmate-bot commented Jul 7, 2017

gitmate-bot commented Jul 14, 2017

gitmate-bot commented Jul 22, 2017

jayvdb Apr 19, 2018

jayvdb Jul 20, 2018

jayvdb Jul 20, 2018

jayvdb Jul 20, 2018

jayvdb Jul 26, 2018

jayvdb Jul 26, 2018

jayvdb Jul 26, 2018

		@@ -102,6 +108,8 @@ def run(self, language: str, minimum_tokens: int=20,

		stdout_output, _ = run_shell_command(arguments)

		os.unlink(temp_filepath)

		@@ -102,6 +109,8 @@ def run(self, language: str, minimum_tokens: int=20,

		stdout_output, _ = run_shell_command(arguments)

		os.remove(tmp_filename)

CPDBear: Fix for crash when many files are given #1429

Are you sure you want to change the base?

CPDBear: Fix for crash when many files are given #1429

Conversation

impmihai commented Feb 13, 2017

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adtac Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

impmihai commented Feb 15, 2017

Makman2 commented Feb 15, 2017 • edited Loading

impmihai commented Feb 16, 2017

Makman2 commented Feb 16, 2017

impmihai commented Jun 1, 2017

Makman2 commented Jun 1, 2017

Makman2 commented Jun 1, 2017

gitmate-bot commented Jun 9, 2017

gitmate-bot commented Jun 15, 2017

gitmate-bot commented Jun 22, 2017

gitmate-bot commented Jun 29, 2017

AsnelChristian commented Jun 30, 2017

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

gitmate-bot commented Jul 7, 2017

gitmate-bot commented Jul 14, 2017

gitmate-bot commented Jul 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adtac Feb 16, 2017 •

edited

Loading

Makman2 commented Feb 15, 2017 •

edited

Loading