add --replace-originals flag to extractcode by maxhbr · Pull Request #1893 · aboutcode-org/scancode-toolkit

maxhbr · 2020-02-10T17:13:21Z

This adds the flag --remove-originals to extractcode. This modifies the behavior to remove the archive that was extracted from the file tree.

Related to: #14

Tasks

Reviewed contribution guidelines
PR is descriptively titled 📑 and links the original issue above 🔗
Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
Run tests locally to check for errors.
Commits are in uniquely-named feature branch and has no merge conflicts 📁

pombredanne

@maxhbr Thank you ++ for this. The --replace-originals makes perfect sense. Let me reformulate the intent and outcome to make sure we are in sync: this forces a deletion of the extraction target and a new re-extraction of any archive?

PS: do not worry too much about all tests failures just now, Azure can be flaky and I will respin some if needed... BUT still there is something which may not be working OK. See https://ci.appveyor.com/project/nexB/scancode-toolkit/builds/30698997/job/ag1rs0yofobgsfd3#L4406

I think that the --replace-originals does may not honor anymore the recursive extraction flag?

Codecov Report

Merging #1893 into develop will increase coverage by 0.08%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1893      +/-   ##
===========================================
+ Coverage    78.94%   79.02%   +0.08%     
===========================================
  Files          131      131              
  Lines        16925    16939      +14     
===========================================
+ Hits         13361    13386      +25     
+ Misses        3564     3553      -11

Impacted Files	Coverage Δ
src/commoncode/codec.py	`94.87% <0.00%> (ø)`	⬆️
src/formattedcode/output_jsonlines.py	`91.30% <0.00%> (ø)`	⬆️
src/commoncode/functional.py	`100.00% <0.00%> (ø)`	⬆️
src/formattedcode/output_json.py	`81.48% <0.00%> (ø)`	⬆️
src/extractcode/uncompress.py	`94.44% <0.00%> (ø)`	⬆️
src/typecode/entropy.py	`96.42% <0.00%> (ø)`	⬆️
src/commoncode/urn.py	`100.00% <0.00%> (ø)`	⬆️
src/cluecode/finder.py	`81.85% <0.00%> (ø)`	⬆️
src/commoncode/command.py	`86.61% <0.00%> (ø)`	⬆️
src/licensedcode/match_hash.py	`84.09% <0.00%> (ø)`	⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1637711...7af031e. Read the comment docs.

maxhbr · 2020-02-10T17:56:24Z

BUT still there is something which may not be working OK. See https://ci.appveyor.com/project/nexB/scancode-toolkit/builds/30698997/job/ag1rs0yofobgsfd3#L4406

There was is an error in the recursion, I have to fix that.

maxhbr · 2020-02-10T18:02:10Z

@pombredanne :

Let me reformulate the intent and outcome to make sure we are in sync: this forces a deletion of the extraction target and a new re-extraction of any archive?

I think we are not in sync here. My goal is to change the behavior in the following way:
instead of extracting to

            a/a.tar.gz
            a/a.txt
            a/a.tar.gz-extract/a/b/a.txt
            a/a.tar.gz-extract/a/b/b.txt
            a/a.tar.gz-extract/a/c/c.txt

I want to extract "in place" and get:

            a/a.txt
            a/a.tar.gz/a/b/a.txt
            a/a.tar.gz/a/b/b.txt
            a/a.tar.gz/a/c/c.txt

maxhbr · 2020-02-10T19:40:41Z

this code breaks at: https://github.com/nexB/scancode-toolkit/blob/16377111febfaef2fd0806200a43cd9f239f7d0a/src/extractcode/__init__.py#L102-L109

maxhbr · 2020-02-10T20:19:35Z

@pombredanne I have updated the description and the intend of this PR. It now no longer tries to do "in place extraction" but instead just removes the original archives after they were extracted.

pombredanne · 2020-02-10T21:23:17Z

@pombredanne :

Let me reformulate the intent and outcome to make sure we are in sync: this forces a deletion of the extraction target and a new re-extraction of any archive?

I think we are not in sync here. My goal is to change the behavior in the following way:
instead of extracting to
            a/a.tar.gz
            a/a.txt
            a/a.tar.gz-extract/a/b/a.txt
            a/a.tar.gz-extract/a/b/b.txt
            a/a.tar.gz-extract/a/c/c.txt
I want to extract "in place" and get:
            a/a.txt
            a/a.tar.gz/a/b/a.txt
            a/a.tar.gz/a/b/b.txt
            a/a.tar.gz/a/c/c.txt

@maxhbr I get it... and I am glad I asked. So this becomes a destructive extraction then, but I can see how this makes things simpler towards #14 alright!

So I wonder if an approach may be instead to do this:

extract things as usual
once done and if you have the arg to replace/overwrite
2.1. remove the location
2.2. move the target to the location

pombredanne · 2020-02-10T21:23:55Z

@pombredanne I have updated the description and the intend of this PR. It now no longer tries to do "in place extraction" but instead just removes the original archives after they were extracted.

Even better! let me review this tomorrow

pombredanne · 2020-02-11T14:48:55Z

                if TRACE:
                    logger.debug('extract:walk: recursing on target: %(target)r' % locals())
-                for xevent in extract(target, kinds, recurse):
+                for xevent in extract(target if remove_originals else localtion, kinds, recurse, remove_originals=remove_originals):


localtion likely would need to be location...

Good catch, i though I had fixed that.

maxhbr · 2020-02-11T23:41:49Z

@pombredanne I have now implemented your suggested way of using the events

pombredanne

@maxhbr Thank you++ for the updates. We are all green 💚 . Please see my minor comments before I merge this!

Signed-off-by: Maximilian Huber <maximilian.huber@tngtech.com>

maxhbr · 2020-02-14T05:58:18Z

@pombredanne the comments are resolved

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

pombredanne

LGTM... Thank you ++ I made a tiny change to the extract() function docstring and I am merging.

JakobSchubert · 2020-02-17T13:07:11Z

Hi @pombredanne,
i wanted to ask if you could trigger the merge, I would love to try out this feature.

Have a nice day,
Jakob

pombredanne · 2020-02-17T13:41:19Z

@JakobSchubert sure thing!
@maxhbr thanks for the hard work 🙇‍♂️

pombredanne reviewed Feb 10, 2020

View reviewed changes

maxhbr force-pushed the maxhbr/addExtractcodeFlag branch from 7062806 to 8a34e1e Compare February 10, 2020 17:51

maxhbr force-pushed the maxhbr/addExtractcodeFlag branch from 8a34e1e to ffd70a9 Compare February 10, 2020 18:03

maxhbr changed the title ~~add --replace-originals flag to extractcode~~ add --remove-originals flag to extractcode Feb 10, 2020

maxhbr force-pushed the maxhbr/addExtractcodeFlag branch from ffd70a9 to 06b5a08 Compare February 10, 2020 20:18

pombredanne reviewed Feb 11, 2020

View reviewed changes

Comment thread src/scancode/api.py

maxhbr force-pushed the maxhbr/addExtractcodeFlag branch from 06b5a08 to c3d6f7b Compare February 11, 2020 23:40

maxhbr changed the title ~~add --remove-originals flag to extractcode~~ add --replace-originals flag to extractcode Feb 11, 2020

pombredanne reviewed Feb 12, 2020

View reviewed changes

Comment thread src/extractcode/extract.py Outdated

Comment thread src/extractcode/extract.py

Comment thread src/extractcode/extract.py Outdated

add --replace-originals flag to extractcode

9fa41f6

Signed-off-by: Maximilian Huber <maximilian.huber@tngtech.com>

maxhbr force-pushed the maxhbr/addExtractcodeFlag branch from c3d6f7b to 9fa41f6 Compare February 12, 2020 13:53

Use the same doc for replace_originals as in CLI

7af031e

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

pombredanne approved these changes Feb 14, 2020

View reviewed changes

pombredanne merged commit 77c0266 into aboutcode-org:develop Feb 17, 2020

pombredanne mentioned this pull request Apr 3, 2020

restore progress reporting to extractcode #1996

Open

                                    + '.' + getattr(extractor, '__name__', ''))
                   if extractor:
-                      yield ExtractEvent(location, target, done=False, warnings=[], errors=[])
+                      yield ExtractEvent(location, target if not replace_originals else location, done=False, warnings=[], errors=[])

Uh oh!

Uh oh!

Conversation

maxhbr commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tasks

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

pombredanne Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

maxhbr Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

maxhbr commented Feb 10, 2020

Uh oh!

maxhbr commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxhbr commented Feb 10, 2020

Uh oh!

maxhbr commented Feb 10, 2020

Uh oh!

pombredanne commented Feb 10, 2020

Uh oh!

pombredanne commented Feb 10, 2020

Uh oh!

pombredanne Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

maxhbr Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maxhbr commented Feb 11, 2020

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maxhbr commented Feb 14, 2020

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

JakobSchubert commented Feb 17, 2020

Uh oh!

pombredanne commented Feb 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maxhbr commented Feb 10, 2020 •

edited

Loading

codecov Bot commented Feb 10, 2020 •

edited

Loading

maxhbr commented Feb 10, 2020 •

edited

Loading