first attempt at supporting `opm build-install` #40

jvanasco · 2016-11-30T21:36:16Z

This is my first attempt at handling #39

I haven't touched Perl in years so apologies in advance.

It seems to work, but there aren't any tests to plug into. I documented new functions inline, and also added docs for existing functions too.

Notes:

The command is invoked as opm build-install as it wraps the build process and installs locally (as requested)
A new core subroutine of do_build_install was created, which handles most of the overall logic
build-install will always remove an existing version, it will not compare revisions.
Variable names were standardized for easier maintenance:
- $rcfile -> $rc_file (same style as $ini_file)
- $data -> $rc_data, $ini_data
- $default_sec -> $rc_default_sec, $ini_default_sec
I noted what the arguments meant to various subroutines, both when invoked and in the subroutine iteself

I tried to leverage existing code, and am not thrilled with my approach (but it does work)

install_target was dropped into two routines: install_target and install_distribution.
install_distribution handles installing the actual distribution file. Most metadata is calculated in install_target or do_build_install
in order to keep track of some package and directory data, I created a secondary context object with init_package_ctx. It's used alongside the existing init_installation_ctx, but is limited to the current package info, and mostly used for specifying the cache directory vs the current directory.

agentzh · 2016-11-30T21:44:19Z

bin/opm

+}
+
+sub do_build_install () {
+=pod


These comments should be comments instead of POD documentation. Also, they should appear before the sub prototype.

agentzh · 2016-11-30T21:44:32Z

bin/opm

+    my $ini_data = read_ini($dist_file);
+    my $ini_default_sec = $ini_data->{default};
+    my $dist_name = $ini_default_sec->{name};
+    my $version = $ini_default_sec->{version};


Bad indentation?

We should use 4 space indentation consistently.

agentzh · 2016-11-30T21:45:20Z

bin/opm

-        my $default_sec = $data->{default};
-        $account = delete $default_sec->{github_account};
+        my ($rc_file, $rc_data) = get_rc_file();
+        my $rc_default_sec = $rc_data->{default};


Please avoid such unrelated changes. I prefer minimal change sets in each PR. Thank you.

Would you mind if I do a PR first that standardizes the variable names? This was a bit of a headache to unwind, because $data and $default_sec would be recycled from the rc file to the ini file.

agentzh · 2016-11-30T21:45:39Z

bin/opm

+	0 = build
+	1 = server-build
+=cut
+    my $_build_type = shift;


Please avoid local variable names led by _.

agentzh · 2016-11-30T21:46:09Z

bin/opm

+	1 = server-build
+=cut
+    my $_build_type = shift;
+    my $server_build = ($_build_type == 1) ? 1 : 0;


Please avoid magical numbers in the build type values.

agentzh · 2016-11-30T21:46:55Z

bin/opm

    if (!$dist_abstract) {
        err "$dist_file: key \"abstract\" not found in the default section.\n";
    }

    check_utf8_field($dist_file, 'abstract', $dist_abstract);

-    my $repo_link = delete $default_sec->{repo_link};
+    my $repo_link = delete $ini_default_sec->{repo_link};


Please avoid such unrelated changes. Thank you.

agentzh · 2016-11-30T21:47:35Z

bin/opm

+}
+
+
+sub install_distribution($$$){


Need a space before { and (.

agentzh · 2016-11-30T21:48:43Z

bin/opm

+	* do_build_install 0
+	* do_remove        0 
+	* do_upgrade       1
+	* do_update        1


Remaining this back-reference list is quite a burden. Better avoid it.

jvanasco · 2016-11-30T22:13:36Z

I'll work on all the changes tomorrow. Sorry for the whitespace issues, my editor didn't pick up Perl and auto-expand the tabs (it does for .py and .pl files)

jvanasco · 2016-12-16T17:40:29Z

I had to do a manual rebase and force-push, but I've got this working again.

agentzh · 2016-12-16T19:35:12Z

bin/opm

+    opm server-build
+    install $dist_name-$version.opm.tar.gz  # psuedocode
+=end comment
+=cut


Better put blank lines around POD directives.

agentzh · 2016-12-16T19:35:28Z

bin/opm

+    	$remove_old = 1;
+    }
+
+	# package_ctx is needed to get the directories right for `install_distribution` 


Bad indentation?

agentzh · 2016-12-16T19:35:57Z

bin/opm

@@ -12,7 +12,7 @@ use File::Find ();
 use File::Path qw( make_path );
 use File::Spec ();
 use Config ();
-use Cwd qw( realpath cwd );
+use Cwd qw( realpath cwd getcwd );


Why import getcwd? Why not just use cwd?

agentzh · 2016-12-16T19:36:35Z

bin/opm

+
+    # phase 2b - `cd `
+    my $versioned_directory = "$dist_name-$version";
+    chdir $versioned_directory;


Should check if chdir fails.

agentzh · 2016-12-16T19:36:54Z

bin/opm

+    	# for `build-install`:
+    	#     *always* `remove_old` if the `meta_file` exists, regardless of version
+    	#     however, `get` will still need to compare version numbers
+    	$remove_old = 1;


We should use 4-space indentations.

agentzh · 2016-12-16T19:37:48Z

bin/opm

+This installs a distribution file (.tar.gz)
+=end comment
+=cut
+sub install_distribution($$$) {


Style: need a space before (.

agentzh · 2016-12-16T19:39:03Z

bin/opm

@@ -1406,7 +1527,7 @@ sub install_target ($$) {
        err "no library files found in $dist_dir/.\n";
    }

-    if ($remove_old) {
+    if ($package_ctx->{remove_old}) {


We should avoid such immediate hash table look ups since when there is a typo in the hash table key here, it is hard to detect and debug.

changed source argument to `install_file`; now a full path (not local) altered `remove_target` to chdir back to the opening directory (not install directory) standardized the directory start logic

jvanasco · 2017-01-06T23:29:01Z

Sorry for the delay in response, I was on vacation.

I've pushed the changes, and will address everything above here:

why getcwd? IIRC, it is supposed to work better with symlinks. I started using it in a few spots where paths seems to switch around.
whitespace and formatting issues. I'm sorry for this. can you recommend a Perl linter or similar for simple stuff like this? It's been a very long time since I've used perl.
the hash table lookup isn't immediate - it happens in another function several hundred lines away. in order for me to use the existing logic of build-install and upload and not duplicate code blocks in-line, I had to decompose some functions into re-usable components (and use a new per-installable context object).

Looking at this with some fresh eyes (and a little more familiarity with Perl), in the current changeset I also altered a few things.

I didn't quite understand how/why some things originally worked with the chdirs -- but now do (I think) and so I tried to more clearly convey what is going on. The functions now note the "start" directory before chdiring, and then explicitly return to that 'start' directory (instead of the "install" directory). This loosely emulates a context manager, and makes chaining or stacking functions a lot easier.

agentzh · 2017-01-09T18:26:58Z

@jvanasco Some comments:

I think it's confusing to use both cwd and getcwd at the same time, especially when they could be different. We need to pick just one consistently.
I never trust tools to automatically format my code though I'm a big fan of creating tools to report any offenders.
You put the hash lookup expression directly in an if statement condition which makes typos in the hash keys extremely hard to detect. I think you should always explicitly test the definedness of the values of those hash table keys before testing the boolean value in the if statement conditions. This makes potential typos in the hash keys nowhere to hide.

jvanasco · 2017-01-09T18:31:34Z

I think getcwd might be better than cwd, because it tries to use the system links and falls back on cwd.
I was hoping you knew of a tool like flake8 in python or jshint for javascript. they simply analyze your code and point out things like whitespace issues, style mistakes from the common formats, possible scope issues, etc. in my experience, they catch 90% of issues in a PR (from me or to me).
Got it on the hash key issue. I will address!

agentzh · 2017-01-09T18:33:18Z

@jvanasco Yes, Perl has similar tools as well. You can dig them up if you like.

We do prepare our own static analysis toolsets for OpenResty's C components and Lua components. But not yet for Perl.

ktalebian · 2019-03-29T21:15:49Z

@agentzh @jvanasco what's the status on this pr?

jvanasco · 2020-11-23T20:36:15Z

@ktalebian I am closing this PR. There were several changes to OpenResty in the months after I first wrote it, which would have required far too much work to rebase or merge with, and I never had time to revisit this since. someone else will need to create this functionality.

agentzh requested changes Nov 30, 2016

View reviewed changes

jvanasco force-pushed the feature-opm_build_install branch from 77c9bc7 to a8bb6db Compare December 16, 2016 17:04

agentzh requested changes Dec 16, 2016

View reviewed changes

jvanasco force-pushed the feature-opm_build_install branch 5 times, most recently from 054e796 to ba45825 Compare January 6, 2017 21:11

jvanasco added 3 commits January 6, 2017 16:17

first attempt at supporting opm build-install

47593b3

whitespace

c53226d

responding to changes from first pr

7f6ec39

jvanasco force-pushed the feature-opm_build_install branch 2 times, most recently from 1a8ae86 to 91c2d25 Compare January 6, 2017 23:10

second round of changes

7e19d31

changed source argument to `install_file`; now a full path (not local) altered `remove_target` to chdir back to the opening directory (not install directory) standardized the directory start logic

jvanasco force-pushed the feature-opm_build_install branch from 91c2d25 to 7e19d31 Compare January 6, 2017 23:14

agentzh mentioned this pull request Dec 30, 2017

[features]Is there any plan to add the opm install #59

Open

jvanasco closed this Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first attempt at supporting `opm build-install` #40

first attempt at supporting `opm build-install` #40

jvanasco commented Nov 30, 2016

agentzh Nov 30, 2016

agentzh Nov 30, 2016

agentzh Nov 30, 2016

agentzh Nov 30, 2016

jvanasco Nov 30, 2016

agentzh Nov 30, 2016

agentzh Nov 30, 2016

agentzh Nov 30, 2016

agentzh Nov 30, 2016

agentzh Nov 30, 2016

jvanasco commented Nov 30, 2016

jvanasco commented Dec 16, 2016

agentzh Dec 16, 2016

agentzh Dec 16, 2016

agentzh Dec 16, 2016

agentzh Dec 16, 2016

agentzh Dec 16, 2016

agentzh Dec 16, 2016

agentzh Dec 16, 2016

jvanasco commented Jan 6, 2017

agentzh commented Jan 9, 2017

jvanasco commented Jan 9, 2017

agentzh commented Jan 9, 2017 •

edited

Loading

ktalebian commented Mar 29, 2019

jvanasco commented Nov 23, 2020

first attempt at supporting opm build-install #40

first attempt at supporting opm build-install #40

Conversation

jvanasco commented Nov 30, 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

jvanasco commented Nov 30, 2016

jvanasco commented Dec 16, 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

jvanasco commented Jan 6, 2017

agentzh commented Jan 9, 2017

jvanasco commented Jan 9, 2017

agentzh commented Jan 9, 2017 • edited Loading

ktalebian commented Mar 29, 2019

jvanasco commented Nov 23, 2020

first attempt at supporting `opm build-install` #40

first attempt at supporting `opm build-install` #40

agentzh commented Jan 9, 2017 •

edited

Loading