remove references to external programs in favor of packages #2172

pstaabp · 2023-08-11T19:59:49Z

This removes the use of the programs mv, cp, rm, mkdir, tar, gzip (some were no longer used). Instead, use functions in perl modules.

This should be tested on a few different platforms. Especially

Hardcopy generation
Course Archiving
Course Unarchiving
Add new course

conf/site.conf.dist

pstaabp · 2023-08-20T20:54:25Z

This is ready for review. Switched to Archive::Zip::SimpleZip like #2163 and improved the creation of tar balls to not use chdir.

Definitely needs some testing in the area listed above.

Alex-Jordan · 2023-08-27T00:12:22Z

Is it relevant that I see a direct use of mkdir in lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm?

pstaabp · 2023-08-28T00:29:09Z

Is it relevant that I see a direct use of mkdir in lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm?

That's the perl mkdir. https://perldoc.perl.org/functions/mkdir

drgrice1 · 2023-10-11T22:02:20Z

This pull request has merge conflicts that need to be resolved.

drgrice1

I haven't testing things yet, but I see some things that need to be fixed in the code.

bin/check_modules.pl

conf/site.conf.dist

lib/HardcopyRenderedProblem.pm

lib/WeBWorK/ContentGenerator/Hardcopy.pm

lib/WeBWorK/Utils/CourseManagement.pm

pstaabp · 2023-10-31T16:23:01Z

@drgrice1 Just made updates.

drgrice1

There are still some things that aren't right.

Also, I don't really think it is worth introducing the Archive::Extract module when the Archive::Tar module covers this and is a core module.

bin/download-OPL-metadata-release.pl

lib/HardcopyRenderedProblem.pm

lib/WeBWorK/ContentGenerator/Hardcopy.pm

lib/WeBWorK/Utils/CourseManagement.pm

pstaabp · 2023-11-10T18:42:20Z

@drgrice1 Fixed these. I've noticed in HardCopy and HardcopyRenderedProblem.pm there are a bunch of file IO that could benefit from using Mojo::File. I've left these as is for now.

drgrice1 · 2023-11-15T17:43:48Z

@pstaabp: Have you tested your latest changes?

pstaabp · 2023-11-20T20:03:48Z

I had tested the course admin features and didn't test the hardcopy creation. Fixed the error.

pstaabp · 2023-12-04T18:28:55Z

@drgrice1 and others, when you get a chance, this is ready to be looked at and tested gain.

drgrice1

Please, just revert to using Archive::Zip in HardcopyRenderedProblem.pm and Hardcopy.pm. Here are the justifications for this request:

Archive::Zip is already NOT using a reference to an external program, and so this change does not fit into this pull request as titled.
Archive::Zip is still needed because Archive::Zip::SimpleZip can not do archive extraction.
Archive::Zip has a nicer API than Archive::Zip::SimpleZip. Look at how much more code it took to implement the same thing in HardcopyRenderedProblem.pm and Hardcopy.pm with Archive::Zip::SimpleZip than with Archive::Zip.
Quite frankly we wouldn't be using Archive::Zip::SimpleZip at all if it weren't for the bug in Archive::Zip with adding symbolic links to zip archives. The zip files created in these two modules don't have symbolic links, so that is irrelevant here.

In fact, please revert all changes to HardcopyRenderedProblem.pm in this pull request. Other than switching from Archive::Zip to Archive::Zip::SimpleZip, you change several cases where the Mojo::File::path method was called into Mojo::File->new calls. You do realize that the path call is just a shorthand for Mojo::File->new don't you? The definition of the path method in Mojo::File is sub path { __PACKAGE__->new(@_) }. The path method could also be imported and used in the CourseManagement.pm file. Do NOT do this in Hardcopy.pm (or any module that derives from ContentGenerator.pm), because that module has another path method that will conflict if the path method is imported from Mojo::File.

conf/site.conf.dist

pstaabp · 2024-01-04T15:59:16Z

reverted to Archive::Zip in Hardcopy.pm and HardcopyRenderedProblem.pm.

drgrice1 · 2024-01-09T03:35:42Z

In fact, please revert all changes to HardcopyRenderedProblem.pm in this pull request. Other than switching from Archive::Zip to Archive::Zip::SimpleZip, you change several cases where the Mojo::File::path method was called into Mojo::File->new calls. You do realize that the path call is just a shorthand for Mojo::File->new don't you? The definition of the path method in Mojo::File is sub path { __PACKAGE__->new(@_) }. The path method could also be imported and used in the CourseManagement.pm file. Do NOT do this in Hardcopy.pm (or any module that derives from ContentGenerator.pm), because that module has another path method that will conflict if the path method is imported from Mojo::File.

This has still not been addressed.

lib/WeBWorK/Utils/CourseManagement.pm

drgrice1

Just a few minor issues to be fixed, and then this is ready to go.

lib/HardcopyRenderedProblem.pm

lib/WeBWorK/Utils/CourseManagement.pm

lib/WeBWorK/ContentGenerator/Hardcopy.pm

and improve tar ball creation without chdir

…leanup. update

drgrice1

I just noticed that you added the usage of the module File::Copy::Recursive, but did not add it to the check_modules.pl script. Make sure to add that.

pstaabp · 2024-02-06T12:44:55Z

Good catch. Updated.

Alex-Jordan · 2024-02-07T22:34:08Z

lib/WeBWorK/Utils/CourseManagement.pm

-		mkdir $courseDir
-			or warn
-			"Failed to create $courseDirName directory '$courseDir': $!. You will have to create this directory manually.\n";
+		eval { Mojo::File($courseDir)->make_path };


Is this an error? Should be Mojo::File->new?

If so and if someone confirms how I should fix it, I can fix it in #2290.

Yes, that should be eval { Mojo::File->new($courseDir)->make_path };. Good catch.

OK, less of a catch though than just debugging when testing #2290.

The dependence on File::Copy::Recursive module was added in openwebwork#2172, but not added to the docker build. Start using node 20 in the docker build. The node 16 install script from nodesource is deprecated, and they have imposed a 60 second install delay when you use it. Fix the ownership on the courses directory and admin course directory when the docker entrypoint runs. Fixing the ownership on the courses directory will not cause any slow down to the execution of that script. It is not a recursive ownership change, just a single directory ownership change which is super fast.

The dependence on File::Copy::Recursive module was added in openwebwork#2172, but not added to the docker build. Start using node 20 in the docker build. The node 16 install script from nodesource is deprecated, and they have imposed a 60 second install delay when you use it. Fix the ownership on the courses directory and admin course directory when the docker entrypoint runs. Fixing the ownership on the courses directory and admin course directory will not cause any slow down to the execution of that script. It is not a recursive ownership change, just two directories for which ownership is changed which is super fast.

drgrice1 reviewed Aug 12, 2023

View reviewed changes

conf/site.conf.dist Show resolved Hide resolved

pstaabp mentioned this pull request Aug 20, 2023

Add zip to archives in File Manager #2163

Merged

pstaabp force-pushed the remove-ext-programs branch 2 times, most recently from 6a56ba8 to d2cd200 Compare August 20, 2023 20:52

pstaabp marked this pull request as ready for review August 20, 2023 20:52

pstaabp force-pushed the remove-ext-programs branch from 64c5a3d to 46242bc Compare October 16, 2023 20:06

drgrice1 reviewed Oct 24, 2023

View reviewed changes

pstaabp force-pushed the remove-ext-programs branch from 46242bc to 69a09ff Compare October 24, 2023 10:49

pstaabp force-pushed the remove-ext-programs branch from ef111d4 to fb3f690 Compare October 31, 2023 16:24

drgrice1 reviewed Nov 3, 2023

View reviewed changes

pstaabp force-pushed the remove-ext-programs branch 3 times, most recently from 2166fe6 to 6f15637 Compare November 10, 2023 18:41

pstaabp force-pushed the remove-ext-programs branch from 6f15637 to 7f3cf8f Compare November 20, 2023 20:03

pstaabp force-pushed the remove-ext-programs branch 2 times, most recently from 87748e4 to abc058e Compare November 20, 2023 20:04

pstaabp force-pushed the remove-ext-programs branch from abc058e to 51abfd5 Compare December 3, 2023 14:59

drgrice1 reviewed Dec 6, 2023

View reviewed changes

conf/site.conf.dist Outdated Show resolved Hide resolved

pstaabp force-pushed the remove-ext-programs branch from 51abfd5 to 62ddf67 Compare January 4, 2024 15:57

Alex-Jordan mentioned this pull request Jan 4, 2024

be able to copy more things from a course when adding a new course #2290

Merged

pstaabp force-pushed the remove-ext-programs branch from 62ddf67 to a0ccbc6 Compare January 4, 2024 17:05

drgrice1 reviewed Jan 9, 2024

View reviewed changes

lib/WeBWorK/Utils/CourseManagement.pm Outdated Show resolved Hide resolved

pstaabp force-pushed the remove-ext-programs branch from a0ccbc6 to e851dda Compare January 10, 2024 19:01

drgrice1 approved these changes Jan 10, 2024

View reviewed changes

lib/HardcopyRenderedProblem.pm Outdated Show resolved Hide resolved

lib/WeBWorK/Utils/CourseManagement.pm Outdated Show resolved Hide resolved

lib/WeBWorK/Utils/CourseManagement.pm Outdated Show resolved Hide resolved

lib/WeBWorK/ContentGenerator/Hardcopy.pm Outdated Show resolved Hide resolved

pstaabp added 11 commits January 25, 2024 06:06

remove references to external programs in favor of packages

4b5d5f8

remove reference to externalPrograms{tar}.

796253d

Switch to using Archive::Zip::SimpleZip

f740bf1

and improve tar ball creation without chdir

Switch rmtree to Mojo::File::remove_tree

811d38a

Some cleanup and switching some file io to Mojo::File

fea8645

Eliminated Archive::Extract in favor of Archive::Tar and some other c…

d9955a4

…leanup. update

fixed minor error.

9482e27

Fix errors on generating zip file and more cleanup

f79ca19

Restore Archive::Zip methods.

d37d776

reverted HardCopyRenderedProblem back to original code.

96d4aca

fix typos and add some space for clarify.

130ba37

pstaabp force-pushed the remove-ext-programs branch from 87c3428 to 130ba37 Compare January 25, 2024 11:07

drgrice1 reviewed Feb 3, 2024

View reviewed changes

add File::Copy::Recursive to checkModules

cea3783

drgrice1 merged commit 41fac6d into openwebwork:develop Feb 7, 2024
1 check passed

Alex-Jordan reviewed Feb 7, 2024

View reviewed changes

drgrice1 mentioned this pull request Feb 22, 2024

Add a patch for the dvisvgm issue observed in https://github.com/openwebwork/pg/issues/1023. #2330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove references to external programs in favor of packages #2172

remove references to external programs in favor of packages #2172

pstaabp commented Aug 11, 2023

pstaabp commented Aug 20, 2023

Alex-Jordan commented Aug 27, 2023

pstaabp commented Aug 28, 2023

drgrice1 commented Oct 11, 2023

drgrice1 left a comment

pstaabp commented Oct 31, 2023 •

edited

Loading

drgrice1 left a comment

pstaabp commented Nov 10, 2023

drgrice1 commented Nov 15, 2023

pstaabp commented Nov 20, 2023

pstaabp commented Dec 4, 2023

drgrice1 left a comment •

edited

Loading

pstaabp commented Jan 4, 2024

drgrice1 commented Jan 9, 2024

drgrice1 left a comment

drgrice1 left a comment

pstaabp commented Feb 6, 2024

Alex-Jordan Feb 7, 2024

Alex-Jordan Feb 7, 2024

drgrice1 Feb 7, 2024

Alex-Jordan Feb 7, 2024

remove references to external programs in favor of packages #2172

remove references to external programs in favor of packages #2172

Conversation

pstaabp commented Aug 11, 2023

pstaabp commented Aug 20, 2023

Alex-Jordan commented Aug 27, 2023

pstaabp commented Aug 28, 2023

drgrice1 commented Oct 11, 2023

drgrice1 left a comment

Choose a reason for hiding this comment

pstaabp commented Oct 31, 2023 • edited Loading

drgrice1 left a comment

Choose a reason for hiding this comment

pstaabp commented Nov 10, 2023

drgrice1 commented Nov 15, 2023

pstaabp commented Nov 20, 2023

pstaabp commented Dec 4, 2023

drgrice1 left a comment • edited Loading

Choose a reason for hiding this comment

pstaabp commented Jan 4, 2024

drgrice1 commented Jan 9, 2024

drgrice1 left a comment

Choose a reason for hiding this comment

drgrice1 left a comment

Choose a reason for hiding this comment

pstaabp commented Feb 6, 2024

Alex-Jordan Feb 7, 2024

Choose a reason for hiding this comment

Alex-Jordan Feb 7, 2024

Choose a reason for hiding this comment

drgrice1 Feb 7, 2024

Choose a reason for hiding this comment

Alex-Jordan Feb 7, 2024

Choose a reason for hiding this comment

pstaabp commented Oct 31, 2023 •

edited

Loading

drgrice1 left a comment •

edited

Loading