Skip to content

Rework check_modules.pl. #2741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 18, 2025

Conversation

drgrice1
Copy link
Member

@drgrice1 drgrice1 commented Jun 13, 2025

The script now uses the settings in the course environment for external programs and to check the database driver (DBD::mysql or DBD::MariaDB). In order for that to work, first the script checks that all of the needed modules are found, load, and are at the sufficient version. That is except for the database driver module. If any are not found, then the script exits with a message stating that it is unable to continue without the required modules.

If all modules are good, then the script checks the database driver module next. For this it "runtime" loads the course environment and its dependencies (as well as a runtime detection of the webwork2 root and lib directories).

This is to address issue #2740.

In addition the following modules are removed that are no longer needed:

  • Array::Utils
  • Email::Sender::Simple - still used but is a dependency of Email::Stuffer and doesn't need to be checked separately
  • HTML::Tagset
  • HTML::Template
  • IO::Socket::SSL
  • Net::LDAPS
  • Net::SMPT
  • Net::SSLeay
  • PadWalker
  • Path::Class
  • Safe - we use our local version (WWSafe.pm) and still check Opcode which it uses
  • Statistics::R::IO
  • Template

npm is added to the list of required applications.

Most of these things were noted in issue #1917.

@drgrice1
Copy link
Member Author

I forgot to mention that the script options have been fixed. Also, the "all" option has been removed. There is no need for an option that is the default behavior.

@drgrice1
Copy link
Member Author

I also forgot to mention that the script also looks up which of dvisvgm or pdf2svg the site is configured to use and only checks for the configured executable.

@somiaj
Copy link
Contributor

somiaj commented Jun 13, 2025

Note, I normally don't install Statistics::R::IO, which is only an optional module (unsure if this is the only optional one), but I don't think the script should fail if that module is missing.

The script now uses the settings in the course environment for external
programs and to check the database driver (DBD::mysql or DBD::MariaDB).
In order for that to work, first the script checks that all of the
needed modules are found, load, and are at the sufficient version. That
is except for the database driver module.  If any are not found, then
the script exits with a message stating that it is unable to continue
without the required modules.

If all modules are good, then the script checks the database driver
module next. For this it "runtime" loads the course environment and its
dependencies (as well as a runtime detection of the webwork2 root and
lib directories).

This is to address issue openwebwork#2740.

In addition the following modules are removed that are no longer needed:

* Array::Utils
* Email::Sender::Simple - still used but is a dependency of Email::Stuffer and doesn't need to be checked separately
* HTML::Tagset
* HTML::Template
* IO::Socket::SSL
* Net::LDAPS
* Net::SMPT
* Net::SSLeay
* PadWalker
* Path::Class
* Safe - we use our local version (WWSafe.pm) and still check Opcode which it uses
* Statistics::R::IO
* Template

`npm` is added to the list of required applications.

Most of these things were noted in issue openwebwork#1917.
@drgrice1 drgrice1 force-pushed the check_modules-rework branch from 75fe00f to 5dd5066 Compare June 13, 2025 15:08
@drgrice1
Copy link
Member Author

I removed Statistics::R::IO. That is no longer a dependency at all, optional or otherwise.

@somiaj
Copy link
Contributor

somiaj commented Jun 13, 2025

What happens if someone uses the -p flag, but modules are missing so the site.conf cannot be loaded? Seems that flag would bypass the initial module test.

@drgrice1
Copy link
Member Author

Yeah, I was going to point that out. If you use the -p flag, then you skip the module check, and there is nothing that can be done about that. So if you use that flag the modules need to be installed first.

@drgrice1
Copy link
Member Author

I don't think anyone really uses the options. They didn't even work before this pull request. The script always just ran all checks.

@somiaj
Copy link
Contributor

somiaj commented Jun 13, 2025

Yea, that is what it appeared. Maybe remove the -p flag, but maybe still allow -m just to check modules. Though if someone uses -p and it errors out, they can just deal with it too.

@drgrice1
Copy link
Member Author

If someone has already checked modules, and know they are all good, then they might just want to only check programs. So I think there is no reason to remove the option.

Copy link
Contributor

@somiaj somiaj left a comment

Choose a reason for hiding this comment

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

Works in my tests.

One thing I noticed is if for some reason a key doesn't exist in $externalPrograms, it does error properly, but the error I found just slightly off, ** latex2pdf not found in $PATH, since there really isn't a bin called latex2pdf, would something like ** latex2pdf is not defined in site.conf be more useful? Not really an issue, just something I noticed in my testing.

@Alex-Jordan
Copy link
Contributor

I was using this site as part of my hunt for the minimal list of modules that PG alone needs. It does list Email::Sender::Simple as a dependency of Email::Stuffer for perl 5.34. But not (for example) perl 5.36. In general it seems dependencies can change based on perl version.

@drgrice1
Copy link
Member Author

@somiaj: Without rather heavily restructuring the script there is no way to do what you are asking.

@drgrice1
Copy link
Member Author

@Alex-Jordan: The Email::Stuffer package lists Email::Sender::Simple as a dependency regardless of the Perl version. See https://metacpan.org/pod/Email::Stuffer.

@drgrice1
Copy link
Member Author

By the way, we don't actually directly use Email::Sender::Simple. We only use it indirectly via Email:Stuffer. So it really doesn't matter if it is a dependency or not. It should not be in check_modules.pl.

@Alex-Jordan
Copy link
Contributor

OK, that's a good reason to remove it.

In general, suppose perl module X depends on module Y. But not consistently across perl versions. Then suppose a person uses cpan or cpanm to install module X and their perl version does not have X depending on Y. I would wonder whether cpan or cpanm would automatically install Y. My guess is no, and whatever the list is here, we should not trim the list based solely on one module being a dependency of another. That's just a comment on how we could think about this list.

@drgrice1
Copy link
Member Author

cpanm does install dependencies. I am not sure for cpan.

@drgrice1
Copy link
Member Author

We recommend cpanm in any case. We don't give instructions for cpan anymore.

@drgrice1
Copy link
Member Author

By the way, we still check for (and directly use) Email::Sender::Transport::SMTP. In actuality that module and Email::Sender::Simple are both part of the same Email::Sender package, and you can't actually install one without the other.

@Alex-Jordan
Copy link
Contributor

That's all good, makes sense. I am just making a larger point beyond the Email packages. For example, at least one perl version has File::Path depending on File::Spec and Carp. But just because we check for File::Path, that does not mean we should drop the checks for those other two modules.

@drgrice1
Copy link
Member Author

I am not sure that I agree. We recommend using cpanm which does take care of installing dependencies. Or even better we recommend using the distribution package manager whenever possible, which is even better at taking care of dependencies.

You mention File::Path and File::Spec. We really don't even need to list those at all. Those are core Perl modules. If you have Perl you have those.

We don't check for Perl, and can't with the check_modules.pl approach because it is itself a Perl script. We could however check the Perl version, but that is something the script has never done.

@drgrice1
Copy link
Member Author

By the way Carp is also a core Perl module.

@drgrice1
Copy link
Member Author

Another reason for not checking dependencies is that we do check that a module loads. If a dependency is missing, then the module will not load, and the reason that the module failed to load will be shown. That reason will state the missing dependency.

Copy link
Contributor

@Alex-Jordan Alex-Jordan left a comment

Choose a reason for hiding this comment

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

I tested this and it works as expected. I removed a perl module and it caught that. I messed up a path in site.conf to a xelatex and it caught that.

@Alex-Jordan Alex-Jordan merged commit 61880f3 into openwebwork:WeBWorK-2.20 Jun 18, 2025
2 checks passed
@drgrice1 drgrice1 deleted the check_modules-rework branch June 18, 2025 17:44
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.

3 participants