Skip to content

Add a cpanfile for the PG repo#691

Merged
pstaabp merged 2 commits intoopenwebwork:developfrom
duffee:PG_cpanfile
Oct 12, 2022
Merged

Add a cpanfile for the PG repo#691
pstaabp merged 2 commits intoopenwebwork:developfrom
duffee:PG_cpanfile

Conversation

@duffee
Copy link
Contributor

@duffee duffee commented Jun 4, 2022

A cpanfile lists the module dependencies for each area
of development (runtime/test/authoring) as determined by Test::CPANfile
and uses the "recommended" tag for suggestions for useful modules.

Permits future "pinning" of versions to installed modules and
doesn't obstruct other workflows.

A cpanfile lists the module dependencies for each area
of development (runtime/test/authoring) as determined by Test::CPANfile
and uses the "recommended" tag for suggestions for useful modules.

Permits future "pinning" of versions to installed modules and
doesn't obstruct other workflows.
@duffee
Copy link
Contributor Author

duffee commented Jun 4, 2022

Re-targeted at the develop branch and isolated from other issues for cleaner discussion.

  1. It's small
  2. You're not forced to use it
  3. Its presence doesn't affect any other workflow
  4. It's very useful
    • It makes installing dependencies easy as cpanm --installdeps .
    • it allows the separation of concerns (run time/test/develop)
    • it lets you specify which versions are required or conflict (e.g. if you didn't have an easy resolution to the SQL::Abstract 2.000001 bug, you could say not to install that particular version)
  5. It's now a standard (appeal to authority here) My last 2 workplaces have required that any new modules used get added to the cpanfile.
  6. It is package manager agnostic. If it finds a module is already installed, it moves on. People who prefer distro packages, should install them first and cpanm or cpm will take care of the rest.
  7. Package managers are not guaranteed to keep up to date with CPAN. Perhaps this is old memories, but I've seen a few modules not receive any love from the distro for 5-10 years.
  8. It plays well with local::lib. Devs who need to chase down bugs on specific perls and specific versions of modules use local::lib to reproduce the reported system.
  9. It degrades gracefully. If the cpanfile is not updated when new modules are brought into PG, you get the same behaviour as before, it complains about missing modules when you hit them and you have to "do" something.
  10. It's easy to update the cpanfile without doing too much work. I used a test in Test::CPANfile to give me a list of all the modules used in the repository that weren't listed in the cpanfile.
  11. It is a simple format. It doesn't require much knowledge to update it.

@drgrice1 is correct that it is an unnecessary file, but points 4, 7 & 8 try to show that its utility justifies adding a small file and the rest are that if you don't find it personally useful, it won't get in your way. So, not required, but recommended.

@duffee
Copy link
Contributor Author

duffee commented Jun 16, 2022

There was a comment today that raised the issue that the cpanfile could be updated without also updating check_modules.pl. I would ask how check_modules.pl is updated currently? Here are some options

technical solutions

  • write a test that emulates Test::CPANfile to check all used modules are included in check_modules.pl
  • write an author test that checks that all required modules in the cpanfile are also in @modulesList in check_modules.pl
  • add an option to check_modules.pl to write a cpanfile

social solution

  • don't accept any PR that changes the cpanfile without the equivalent change in check_modules.pl

@dlglin
Copy link
Member

dlglin commented Jun 16, 2022

My suggestion is a variation of your third option: split off the list of dependencies from the check_modules script.

Create a new file somewhere in the repo that contains all of the ww dependencies, along with preferred methods for installing them on any supported OS.

check_modules would then query this file to determine missing packages. It would also allow check_modules to be expanded into a script that installs the dependencies (or this could be a separate script). It would also make it easier for people to write custom scripts to check and/or install dependencies.

We should then be able to set up a GitHub action to generate the cpanfile any time the dependencies file is updated.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be some modules that are missing from the cpanfile that this pull request adds, that are used by PG. For example Encode, GD, and Class::Accessor. There are probably others. Look at the modules list in defaults.conf of the webwork2 code to see all of the modules that are loaded for PG.

Add modules listed in CPAN in webwork2/conf/defaults.config
Add dependancies of Rserve as "recommends"
Remove URI::Escape as it is not actually used by Applet.pm
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will approve this for now.

However, in light of my observation in #702 regarding prop isa of the Test2::V0 object builder not being in version 0.000129 of the Test2::Suite package, it seems that this should also have version specifications for all packages. At least minimums should be given. I am not sure what those need to be, and that may take some work to figure out. So we can leave this for later for now.

For Test2::V0 with the changes in my pull request to your branch for #702 the tests work with version 0.000129 (and maybe earlier), but with the prop isa and check_isa you used, at least version 0.000139 is needed.

@drgrice1
Copy link
Member

Note, that although I have approved this, this should not be merged in its current state until after #702 is fixed and merged. It still requires Test2::V0 (and related modules).

@pstaabp pstaabp merged commit d4ac376 into openwebwork:develop Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants