add a new command `opm clean` #22

xiangnanscu · 2016-10-05T02:18:56Z

to delete folder and .gz file created by opm build or opm upload

agentzh · 2016-10-05T03:00:05Z

@Pronan You patch looks wrong to me. I don't think your code even runs:

$ perl -c bin/opm
Global symbol "$root_dir" requires explicit package name (did you forget to declare "my $root_dir"?) at bin/opm line 2203.
Global symbol "$root_dir" requires explicit package name (did you forget to declare "my $root_dir"?) at bin/opm line 2204.

fully tested and passed

xiangnanscu · 2016-10-05T04:07:41Z

@agentzh Sorry, chun ge, I fixed it. This time this commit is fully tested and passed. lua-resty-query is just uploaded and cleaned via sudo opm build && sudo opm upload && sudo opm dist-clean

bungle · 2016-10-05T04:18:20Z

It only cleans the latest. Should it clean the previously created versions as well?

xiangnanscu · 2016-10-05T04:24:23Z

@bungle do you mean change my $root_dir = "$dist_name-$version"; to my $root_dir = "$dist_name-*"; so the final string will be like rm -rf ./lua-resty-datetime-* ?

bungle · 2016-10-05T07:58:19Z

@Pronan, I mean that maybe that should be considered as well. Not sure if that should be a default or should we introduce a command line argument.

# clean latest / current or all?
opm dist-clean

# clean current
opm dist-clean current    # or
opm dist-clean --current  # or
opm dist-clean -c


# clean all
opm dist-clean all        # or
opm dist-clean --all      # or
opm dist-clean -a

Or something? Also I'm not sure if it should be called dist-clean or just clean (currently only server-build is two words - I have seen @agentzh suggesting dist-clean on #21, though).

agentzh · 2016-10-05T18:48:55Z

@Pronan Sorry, I changed my mind a bit. It seems like opm clean dist is better. So that we can also have opm clean cache in the future ;) What do you think?

I agree that opm clean dist should also clean up tarballs and directories belonging to the older versions. But I do think @bungle's current argument or --current option are overkill.

I think it's good enough to just cleanup whatever looks like our leftover. Basically we can iterate through the current working directory, checking each file or directory, looking for

a directory whose name matches the pattern /^\Q$name\E-[.\w]*\d[.\w]*$/ and also containing a file dist.ini in itself; and
a file whose name matches the pattern /^\Q$name\E-[.\w]*\d[.\w]*\.tar\.gz$/.

And remove them all.

To iterate through the cwd, we can just use the opendir, readdir, and closedir sequence. We do have such examples in the existing opm code base :)

What's your opinion?

e.g. if your package name is `lua-resty-foo`, now with option `dist`, i.e. `opm clean dist` will delete all folders like `lua-resty-foo-1.0` and files like `lua-resty-foo-1.0.tar.gz`

xiangnanscu · 2016-10-06T07:07:44Z

@agentzh hello, chun ge. I agree with you.PR is tested and passed with your regex pattern.

sorry, I've tested several version, and commited a wrong vesion. this one is correct

agentzh · 2016-10-06T19:19:09Z

bin/opm

+        opendir(my $dh, './') || die "Can't opendir";
+        my @dots = readdir($dh);
+        foreach my $x (@dots) {
+            if(-d $x){


Style: need a space after if and also before {. Please keep the coding style consistent with the rest of the code base. Thanks!

agentzh · 2016-10-06T19:19:25Z

bin/opm

+                    shell "rm -rf $x";
+                    say 'delete:',$x;
+                }
+            }else{


Style: need a space before and after else.

agentzh · 2016-10-06T19:20:10Z

bin/opm

+            if(-d $x){
+                if ($x =~ /^\Q$dist_name\E-[.\w]*\d[.\w]*$/){
+                    shell "rm -rf $x";
+                    say 'delete:',$x;


Better avoid using say to be consistent with the rest of the code base. Also need a space after the comma (,).

agentzh · 2016-10-06T19:21:10Z

bin/opm

+        my $data = read_ini($dist_file);
+        my $default_sec = $data->{default};
+        my $dist_name = $default_sec->{name};
+        opendir(my $dh, './') || die "Can't opendir";


Error message in case of opendir failures is not descriptive enough. Please see similar places in this file for examples.

agentzh · 2016-10-06T19:21:32Z

bin/opm

+    if (@_ == 0) {
+        err "no clean option specified.\n";
+    }
+    if ($_[0]  eq 'dist') {


Style: 2 spaces before eq while only one is needed.

Also, need a blank line before this if statement.

agentzh · 2016-10-06T19:23:10Z

bin/opm

+                }
+            }
+        }
+        closedir $dh;


Need error handling when closedir returns an error.

agentzh · 2016-10-06T19:23:34Z

bin/opm

+                }
+            }else{
+                if ($x =~ /^\Q$dist_name\E-[.\w]*\d[.\w]*\.tar\.gz$/){
+                    shell "rm -rf $x";


We should use the Perl builtin function unlink to remove a single file. rm -rf is overkill here.

agentzh · 2016-10-06T19:23:52Z

bin/opm

+        my $default_sec = $data->{default};
+        my $dist_name = $default_sec->{name};
+        opendir(my $dh, './') || die "Can't opendir";
+        my @dots = readdir($dh);


dots is a bad name. Maybe entities is better?

agentzh · 2016-10-06T19:24:13Z

bin/opm

+        my $dist_name = $default_sec->{name};
+        opendir(my $dh, './') || die "Can't opendir";
+        my @dots = readdir($dh);
+        foreach my $x (@dots) {


x is a bad name. we need more descriptive names like entity, I think.

Oh, BTW, I think you can just use for here instead of foreach.

agentzh · 2016-10-06T19:26:02Z

bin/opm

+        }
+        closedir $dh;
+    }else{
+        err "invalid clean option: ".$_[0];


Again, we need spaces around the . operator. I think the error message unrecognized argument for clean is better. Also, we should give a list of recognized clean arguments here as well.

agentzh · 2016-10-06T19:29:54Z

Thanks for your work on this! Please address the issues in my latest round of review :) Thanks!

agentzh · 2016-10-06T19:38:52Z

bin/opm

+        foreach my $x (@dots) {
+            if(-d $x){
+                if ($x =~ /^\Q$dist_name\E-[.\w]*\d[.\w]*$/){
+                    shell "rm -rf $x";


Oh, forgot to mention that, we also need to test if there is an dist.ini file in this directory before removing it recursively. rm -rf is scary and we can never be too cautious here ;)

Do you mean if there is a dist.ini file in it, it should not be removed even if it matches the regex?

I mean, if there is no dist.ini in a directory, then we should not remove it. Since the dist directory generated by opm must have a dist.ini file in it (unless there is a failure in the middle of the previous opm build run).

agentzh · 2016-10-06T19:43:50Z

And...we also need to update the usage text in the program as well as the documentation for this new feature BTW :)

xiangnanscu

hello, chun ge.

xiangnanscu · 2016-10-07T06:00:29Z

bin/opm

+        }
+        closedir $dh;
+    }else{
+        err "invalid clean option: ".$_[0];


xiangnanscu · 2016-10-07T06:01:02Z

bin/opm

+    if (@_ == 0) {
+        err "no clean option specified.\n";
+    }
+    if ($_[0]  eq 'dist') {


xiangnanscu · 2016-10-07T06:01:26Z

bin/opm

+        my $dist_file = "dist.ini";
+        my $data = read_ini($dist_file);
+        my $default_sec = $data->{default};
+        my $dist_name = $default_sec->{name};


a little hard for me. wait

xiangnanscu · 2016-10-07T06:01:35Z

bin/opm

+        my $data = read_ini($dist_file);
+        my $default_sec = $data->{default};
+        my $dist_name = $default_sec->{name};
+        opendir(my $dh, './') || die "Can't opendir";


xiangnanscu · 2016-10-07T06:01:42Z

bin/opm

+        my $default_sec = $data->{default};
+        my $dist_name = $default_sec->{name};
+        opendir(my $dh, './') || die "Can't opendir";
+        my @dots = readdir($dh);


xiangnanscu · 2016-10-07T06:02:38Z

bin/opm

+                }
+            }else{
+                if ($x =~ /^\Q$dist_name\E-[.\w]*\d[.\w]*\.tar\.gz$/){
+                    shell "rm -rf $x";


xiangnanscu · 2016-10-07T06:02:45Z

bin/opm

+                }
+            }
+        }
+        closedir $dh;


xiangnanscu · 2016-10-07T06:02:51Z

bin/opm

+            }
+        }
+        closedir $dh;
+    }else{


xiangnanscu · 2016-10-07T06:07:27Z

bin/opm

+        foreach my $x (@dots) {
+            if(-d $x){
+                if ($x =~ /^\Q$dist_name\E-[.\w]*\d[.\w]*$/){
+                    shell "rm -rf $x";


Do you mean if there is a dist.ini file in it, it should not be removed even if it matches the regex?

xiangnanscu · 2016-10-07T06:17:10Z

bin/opm

+        my $dist_file = "dist.ini";
+        my $data = read_ini($dist_file);
+        my $default_sec = $data->{default};
+        my $dist_name = $default_sec->{name};


name is a varible not a string literal, right? but I can't find where it is defined in do_build.

agentzh · 2016-10-07T18:56:10Z

@Pronan $default_sec->{name} is reading the data from the dist.ini file so it could be empty or bad. Just look at other similar places in the opm source file for examples of checking validity of this value.

xiangnanscu · 2016-10-08T13:02:49Z

@agentzh Hello, Spring big brother, all done and test passed.

agentzh · 2016-10-08T17:12:29Z

bin/opm

+            }
+        }
+        closedir $dh
+            or err "failed to close directory $dh!\n";


We usually do not use ! at the end of our error string messages. Instead, you should use the special variable $! here to output the system error string for the failed closedir operation.

agentzh · 2016-10-08T17:13:31Z

bin/opm

+    my @valid_options = ('dist');
+
+    if (@_ == 0) {
+        err "no clean option specified.\n";


I think "dist" is a clean argument instead of an option. In the CLI terminology, options are usually something like --cwd and --help.

agentzh · 2016-10-08T17:13:54Z

bin/opm

+        {
+            err "$dist_file: bad dist name: $dist_name\n";
+        }
+        opendir(my $dh, './') 


Need a blank line after the if statement block.

agentzh · 2016-10-08T17:14:30Z

bin/opm

+            err "$dist_file: bad dist name: $dist_name\n";
+        }
+        opendir(my $dh, './') 
+            or err "failed to open directory './'!\n";


We need to use : $! instead of ! here. See other similar places. Be careful ;)

agentzh · 2016-10-08T17:14:47Z

bin/opm

+        {
+            err "$dist_file: bad dist name: $dist_name\n";
+        }
+        opendir(my $dh, './') 


./ looks strange to me. You can just use . here.

agentzh · 2016-10-08T17:16:16Z

bin/opm

+
+                    if (-e "$entity/dist.ini") {
+                        shell "rm -rf $entity";
+                        print "delete folder: $entity\n";


"delete" => "removed",
"folder" => "directory",
":" => ""

agentzh · 2016-10-08T17:16:30Z

bin/opm

+
+                if ($entity =~ /^\Q$dist_name\E-[.\w]*\d[.\w]*\.tar\.gz$/) {
+                    unlink $entity;
+                    print "delete file: $entity\n";


"delete" => "removed"
": " => ""

agentzh · 2016-10-08T17:17:34Z

bin/opm

+        }
+
+        if (length $dist_name < 3
+            || $dist_name =~ /[^-\w]|^(?:nginx|luajit|resty|openresty|opm|restydoc.*|ngx_.*|.*-nginx-module)$/i)


Because this check is also used elsewhere. Maybe we should introduce a small Perl sub to avoid code duplication here? Like sub is_valid_dist_name?

agentzh · 2016-10-08T17:18:43Z

bin/opm

@@ -2195,6 +2198,57 @@ sub rebase_path ($$$) {
    return undef;
 }

+sub do_clean {
+    my @valid_options = ('dist');


We won't have many different kinds of arguments, especially when we only have one right now. No need to introduce a Perl array here. It is just referenced once and simply adds line noises IMHO :)

agentzh · 2016-10-08T17:19:20Z

bin/opm

+        closedir $dh
+            or err "failed to close directory $dh!\n";
+    } else {
+        err "unrecognized argument for clean: $_[0]. recognized clean arguments are: " . join(", ", @valid_options);


This line exceeds 80 columns a lot, I'm afraid ;)

xiangnanscu · 2016-10-09T00:04:17Z

Hello, chun ge. All done but I'm not sure how to merge my two PR so I did nothing for that.

agentzh · 2016-10-09T02:47:17Z

@Pronan You can just git cherry-pick the commit(s) in your branch for PR #26 over to your branch for this PR.

xiangnanscu · 2016-10-09T03:02:14Z

@agentzh hello, Should I do this in local computer or just on github webpage? I clone my fork pronan/opm to my comupter, but I found it is same with yours. I do change two files. I don't know why.

agentzh · 2016-10-09T03:43:13Z

@Pronan You do git cherry-pick in your local branches and when your local branch is ready, you just git push. Unlike SVN, one seldom or never does such operations directly on the remote git branches.

agentzh · 2016-10-09T03:44:18Z

@Pronan Do not use GitHub's web UI to make changes. That tends to suck a lot.

xiangnanscu · 2016-10-09T03:48:59Z

@agentzh Saddly, I was using github web UI to make changes.It sucks. Now I'm working on local branch.

agentzh · 2016-10-09T05:28:11Z

cool.

agentzh · 2016-11-01T03:00:15Z

This patch has been updated and merged with #26 to master.

add a new command opm dist-clean

88584ae

to delete folder and .gz file created by opm build or opm upload

Declare path varibles

a52657e

fully tested and passed

change to opm clean

89540f9

e.g. if your package name is `lua-resty-foo`, now with option `dist`, i.e. `opm clean dist` will delete all folders like `lua-resty-foo-1.0` and files like `lua-resty-foo-1.0.tar.gz`

xiangnanscu changed the title ~~add a new command opm dist-clean~~ add a new command opm clean Oct 6, 2016

Correct version.

612295e

sorry, I've tested several version, and commited a wrong vesion. this one is correct

agentzh requested changes Oct 6, 2016

View reviewed changes

agentzh reviewed Oct 6, 2016

View reviewed changes

xiangnanscu commented Oct 7, 2016

View reviewed changes

try to fix style and others

6ffbf8b

add name validation and file check

0d20278

agentzh requested changes Oct 8, 2016

View reviewed changes

agentzh mentioned this pull request Oct 8, 2016

add opm clean dist guide #26

Closed

add helper and fix others

28a1d30

xiangnanscu mentioned this pull request Oct 9, 2016

All in one #27

Closed

agentzh closed this Nov 1, 2016

add a new command opm clean #22

add a new command opm clean #22

Conversation

xiangnanscu commented Oct 5, 2016

agentzh commented Oct 5, 2016

xiangnanscu commented Oct 5, 2016

bungle commented Oct 5, 2016

xiangnanscu commented Oct 5, 2016 • edited Loading

bungle commented Oct 5, 2016 • edited Loading

agentzh commented Oct 5, 2016 • edited Loading

xiangnanscu commented Oct 6, 2016

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

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

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

Choose a reason for hiding this comment

agentzh commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agentzh commented Oct 6, 2016

xiangnanscu left a 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

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

agentzh commented Oct 7, 2016

xiangnanscu commented Oct 8, 2016

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

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

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

xiangnanscu commented Oct 9, 2016

agentzh commented Oct 9, 2016

xiangnanscu commented Oct 9, 2016

agentzh commented Oct 9, 2016

agentzh commented Oct 9, 2016

xiangnanscu commented Oct 9, 2016

agentzh commented Oct 9, 2016

agentzh commented Nov 1, 2016

add a new command `opm clean` #22

add a new command `opm clean` #22

xiangnanscu commented Oct 5, 2016 •

edited

Loading

bungle commented Oct 5, 2016 •

edited

Loading

agentzh commented Oct 5, 2016 •

edited

Loading