refactoring the Release script #381

andreeap · 2015-01-19T17:08:31Z

This is from Friday, still learning.

oalders · 2015-01-19T17:26:48Z

lib/MetaCPAN/Script/Release.pm

@@ -466,7 +404,7 @@ sub import_tarball {
        }
    }
    if (@provides) {
-        $release->provides( [ sort @provides ] );
+        $release->provides( \@provides );


Why did you remove the sort?

In the new version I did not, this why I said I messed up with git, I solved some merge conflicts and deleted some things that shouldn't have been deleted. The sort stays.

andreeap · 2015-01-20T06:35:59Z

I have not put 'isa' everywhere because I am not sure how to add their type.

oalders · 2015-01-21T03:56:55Z

cpanfile

@@ -95,6 +95,7 @@ requires 'MooseX::ClassAttribute';
 requires 'MooseX::Getopt';
 requires 'MooseX::Getopt::Dashes';
 requires 'MooseX::Getopt::OptionTypeMap';
+requires 'MooseX::StrictConstructor';


When you add a new module, make sure you run carton and then commit the changes to cpanfile.snapshot so that we can mimic your installed modules in production.

Then if we change how archives are extracted in the future it only has to be done in one place. MC::Model::Archive deliberately does not clean up a directory given to it, we don't know the caller's intent, but it does use a temp directory by default. Someone can add an option later if it's needed.

Needed to allow the model to make the MC::Document::Release.

"document" is more descriptive of what it is than "release" in the context of the release script.

In the context of the release script, $release_model is redundant.

This will be used by MC::Model::Archive and the like to coerce relative paths into absolute to protect against chdir'ing.

This protects against chdir'ing. As more lazy initialization is used, this becomes more of a concern. File::Find, for example, is a problem.

This lets it derive its own attributes, easier to instantiate. It also gives it everything necessary to create an MC::Document::Release which is where we're heading next. Note: The fixed version was never used before. Now it is.

I'm not happy that building it also indexes it, but I don't know how to instanciate a document without also adding it to the index.

coveralls · 2015-02-19T02:01:46Z

Coverage increased (+0.22%) to 88.81% when pulling 40b6f88 on andreeap:indexing into 141117f on CPAN-API:master.

coveralls · 2015-02-19T02:01:48Z

Coverage increased (+0.22%) to 88.81% when pulling 40b6f88 on andreeap:indexing into 141117f on CPAN-API:master.

More efficient when it's called multiple times, and it's going to be as import_archive is split up further.

Bring it all together into one method for getting the modules.

coveralls · 2015-02-19T06:01:07Z

Coverage increased (+0.17%) to 88.75% when pulling cace0e5 on andreeap:indexing into 141117f on CPAN-API:master.

schwern · 2015-02-19T19:07:54Z

I'm done refactoring. It's far from perfect, I was very rote and conservative and leaned heavily on the existing tests. There's some attribute name inconsistencies between Document::Release, Script::Release and Model::Release it would be nice to resolve.

This is enough to start hooking in Gitpan.

After all that code being moved around between the two modules.

coveralls · 2015-02-19T22:08:15Z

Coverage increased (+0.22%) to 88.8% when pulling f3de0bc on andreeap:indexing into 141117f on CPAN-API:master.

oalders · 2015-02-20T04:25:49Z

I don't see anything here which causes me any concerns. Also, I feel like this is a nice cleanup over the existing code. It's way less confusing now that things are more compartmentalized. I'd like to leave this open for a day or two while we get some more feedback, but I also don't want to leave this open for very long. If there's no dissent, I say we merge this on Monday (Feb 23).

@andreeap how much have you tested this? If you haven't done a full re-index via this branch, now would be a good time to kick that off.

andreeap · 2015-02-20T11:15:31Z

I have tested this on my VM and everything seemed fine at indexing, but I was indexing more than one release and when I tried with just one I noticed that for every release I will get 3 x D release: Extracting archive to filesystem which means it will try to extract the archive for so many times. This should really happen?
I started a full re-index and I got a few of these(but from what I have seen are just the usual errors and because of the changes some sound different)


2015/02/20 10:26:06 I release: Processing /home/metacpan/CPAN/authors/id/R/RI/RIK/NIS-a2.tar.Z
No handler available for type 'application/x-compress' at /home/metacpan/carton/metacpan-api/lib/perl5/Archive/Any.pm line 52.
2015/02/20 10:26:06 F release: Attribute (_extractor) does not pass the type constraint because: Validation failed for 'Archive::Any' with value undef at reader MetaCPAN::Model::Archive::_extractor (defined at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Model/Archive.pm line 69) line 8.
    MetaCPAN::Model::Archive::_extractor('MetaCPAN::Model::Archive=HASH(0x60bbf50)') called at /home/metacpan/carton/metacpan-api/lib/perl5/x86_64-linux/Moose/Meta/Method/Delegation.pm line 92
    MetaCPAN::Model::Archive::is_impolite('MetaCPAN::Model::Archive=HASH(0x60bbf50)') called at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Model/Release.pm line 128
    MetaCPAN::Model::Release::_build_archive('MetaCPAN::Model::Release=HASH(0xfafe990)') called at reader MetaCPAN::Model::Release::archive (defined at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Model/Release.pm line 21) line 7
    MetaCPAN::Model::Release::archive('MetaCPAN::Model::Release=HASH(0xfafe990)') called at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Model/Release.pm line 216
    MetaCPAN::Model::Release::__ANON__() called at /home/metacpan/carton/metacpan-api/lib/perl5/Log/Contextual/Router.pm line 138
    Log::Contextual::Router::handle_log_request('Log::Contextual::Router=HASH(0x3992b10)', 'exporter', 'Log::Contextual', 'caller_package', 'MetaCPAN::Model::Release', 'caller_level', 1, 'message_level', 'debug', 'message_sub', 'CODE(0xfafec90)', 'message_args', 'ARRAY(0x5186bb8)') called at /home/metacpan/carton/metacpan-api/lib/perl5/Log/Contextual.pm line 130
    Log::Contextual::__ANON__('CODE(0xfafec90)') called at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Model/Release.pm line 216
    MetaCPAN::Model::Release::_build_files('MetaCPAN::Model::Release=HASH(0xfafe990)') called at reader MetaCPAN::Model::Release::files (defined at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Model/Release.pm line 63) line 7
    MetaCPAN::Model::Release::files('MetaCPAN::Model::Release=HASH(0xfafe990)') called at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Script/Release.pm line 196
    MetaCPAN::Script::Release::import_archive('MetaCPAN::Script::Release=HASH(0x5eef620)', '/home/metacpan/CPAN/authors/id/R/RI/RIK/NIS-a2.tar.Z') called at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Script/Release.pm line 163
    MetaCPAN::Script::Release::try {...} () called at /home/metacpan/carton/metacpan-api/lib/perl5/Try/Tiny.pm line 81
    eval {...} called at /home/metacpan/carton/metacpan-api/lib/perl5/Try/Tiny.pm line 72
    Try::Tiny::try('CODE(0xfb01e00)', 'Try::Tiny::Catch=REF(0x93609b0)') called at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Script/Release.pm line 166
    MetaCPAN::Script::Release::run('MetaCPAN::Script::Release=HASH(0x5eef620)') called at /home/metacpan/carton/metacpan-api/lib/perl5/x86_64-linux/Class/MOP/Method/Wrapped.pm line 50
    Class::MOP::Method::Wrapped::__ANON__('MetaCPAN::Script::Release=HASH(0x5eef620)') called at /home/metacpan/carton/metacpan-api/lib/perl5/x86_64-linux/Class/MOP/Method/Wrapped.pm line 91
    MetaCPAN::Script::Release::run('MetaCPAN::Script::Release=HASH(0x5eef620)') called at /home/metacpan/metacpan-api/bin/../lib/MetaCPAN/Script/Runner.pm line 32
    MetaCPAN::Script::Runner::run('MetaCPAN::Script::Runner') called at /home/metacpan/metacpan-api/bin/metacpan line 21

coveralls · 2015-02-20T13:25:01Z

Coverage increased (+0.15%) to 88.73% when pulling f0bea9c on andreeap:indexing into 141117f on CPAN-API:master.

schwern · 2015-02-20T18:31:28Z

No, it's only being extracted once, but it's being asked to be extracted multiple times.

The logging for extract is in Model::Release->extract(), but the actual extraction is done in Model::Archive::extract(). Model::Archive->extract() will only extract once, thereafter it will do nothing. This was done so the expensive extract didn't have to be called predicatively, it can now be called just in time. I avoided putting the log message in Module::Archive because it would make initializing Module::Archive more involved and make unit testing harder.

Model::Release could check Model::Archive->_has_extracted but that would defeat the whole point of encapsulating the process in Model::Archive.

It might make more sense to add a log initialization function to a test helper module.

I'll get on fixing this.

oalders · 2015-02-24T02:15:14Z

I'm going to merge this now, since overly verbose logging seems like a secondary concern.

refactoring the Release script

schwern · 2015-02-24T02:49:59Z

Sorry, I got distracted.

haarg added the in progress label Jan 19, 2015

oalders reviewed Jan 19, 2015
View reviewed changes

andreeap force-pushed the indexing branch 7 times, most recently from 4b9e2a8 to 5436277 Compare January 20, 2015 19:02

oalders reviewed Jan 21, 2015
View reviewed changes

andreeap force-pushed the indexing branch 3 times, most recently from 9e3769b to f7fc9dc Compare January 21, 2015 18:50

schwern added 10 commits February 18, 2015 13:17

Using subdir() to make a file is a no-no.

d11a414

Extract building the dependency list from MC::Script::Release

f7a3fcc

Needed to allow the model to make the MC::Document::Release.

Fix perlcritic's whinging about trailng comma and double quotes.

b68f129

Refactor $release -> $document

3794ec9

"document" is more descriptive of what it is than "release" in the context of the release script.

Refactor $release_model -> $model

946ef4e

In the context of the release script, $release_model is redundant.

Provide absolute file and dir types.

49d0891

This will be used by MC::Model::Archive and the like to coerce relative paths into absolute to protect against chdir'ing.

Store archive paths as absolute.

69d2016

This protects against chdir'ing. As more lazy initialization is used, this becomes more of a concern. File::Find, for example, is a problem.

Move building the release document into the release model.

40b6f88

I'm not happy that building it also indexes it, but I don't know how to instanciate a document without also adding it to the index.

schwern added 3 commits February 18, 2015 21:25

Change MC::Model::Release to return an arrayref of files.

06b4727

More efficient when it's called multiple times, and it's going to be as import_archive is split up further.

Extract reading the modules from meta provides.

ecb4d4a

Extract getting modules from files.

cace0e5

Bring it all together into one method for getting the modules.

andreeap changed the title ~~add Tarball model~~ refactoring the Release script Feb 19, 2015

Tidy up the modules used by Model::Release and Script::Release.

f3de0bc

After all that code being moved around between the two modules.

print the name instead of a reference

f0bea9c

oalders added a commit that referenced this pull request Feb 24, 2015

Merge pull request #381 from andreeap/indexing

2c66076

refactoring the Release script

oalders merged commit 2c66076 into metacpan:master Feb 24, 2015

haarg removed the in progress label Feb 24, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactoring the Release script #381

refactoring the Release script #381

andreeap commented Jan 19, 2015

oalders Jan 19, 2015

andreeap Jan 19, 2015

andreeap commented Jan 20, 2015

oalders Jan 21, 2015

coveralls commented Feb 19, 2015

coveralls commented Feb 19, 2015

coveralls commented Feb 19, 2015

schwern commented Feb 19, 2015

coveralls commented Feb 19, 2015

oalders commented Feb 20, 2015

andreeap commented Feb 20, 2015

coveralls commented Feb 20, 2015

schwern commented Feb 20, 2015

oalders commented Feb 24, 2015

schwern commented Feb 24, 2015

refactoring the Release script #381

refactoring the Release script #381

Conversation

andreeap commented Jan 19, 2015

oalders Jan 19, 2015

Choose a reason for hiding this comment

andreeap Jan 19, 2015

Choose a reason for hiding this comment

andreeap commented Jan 20, 2015

oalders Jan 21, 2015

Choose a reason for hiding this comment

coveralls commented Feb 19, 2015

coveralls commented Feb 19, 2015

coveralls commented Feb 19, 2015

schwern commented Feb 19, 2015

coveralls commented Feb 19, 2015

oalders commented Feb 20, 2015

andreeap commented Feb 20, 2015

coveralls commented Feb 20, 2015

schwern commented Feb 20, 2015

oalders commented Feb 24, 2015

schwern commented Feb 24, 2015