Skip to content

Move the WeBWorK::PG module to PG and improve PG's local configuration. #709

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 8 commits into from
Oct 12, 2022

Conversation

drgrice1
Copy link
Member

@drgrice1 drgrice1 commented Aug 20, 2022

Remove the ALWAYS_SHOW_HINT_PERMISSION_LEVEL and ALWAYS_SHOW_SOLUTION_PERMISSION_LEVEL settings. Whether hints and solutions are shown is now determined by the front end.

The pg flag/variable $showHint has been removed. The front end has complete control on when hints are shown. Not the problem author.

Also the options to use knowls for solutions and hints have been removed. Always use knowls for solutions and hints.

Showing the various pieces of debugging information for a problem is now controlled with translation options. The front end is responsible for converting permissions into these debugging options. The VIEW_PROBLEM_DEBUGGING_INFO special pg environment variable was removed and is one of these debugging options.

Many unnecessary PG environment variables have been removed. One of these is the session key.

Remove reduced scoring entirely from PG. That is the responsibility of the front end to implement.

Pg now has no dates and and no permissions.

The color.c file has been removed. The method in that file has been reimplemented in Chromatic.pm using perl. It is perhaps a bit slower, but not significantly in the scope of rendering a problem. This can be tested with the problems
Library/NAU/setGraphTheory/FindChromaticNumber.pg,
Library/NAU/setGraphTheory/FindRoomColor.pg, and
Library/NAU/setGraphTheory/RoomSchedule.pg
which happen to be all known problems that use the Chromatic module.

The image generator has been rewritten to remove the dvipng_align hack and to allow the usage of sqlite for a database backend for storing dvipng depths.

Note that PGalias no longer uses the course name, user login, set number, and problem number for generating resource UUID's. It now only uses the problem seed, psvn, and problemUUID. It is the caller's responsibility of ensuring that problemUUID guarantees the necessary uniqueness.

Some unit tests were added for this.

The t/macros/load_macros.t file tests that all macros load without errors or warnings. There is one macro that will not work in this test and it is skipped. That is the answerDiscussion.pl macro. It requires a file that is not part of the PG distribution. It also has invalid defined(@array) calls.

Warnings found in other macro files were fixed.

The PG_module_list.pl macro was deleted. That is clearly obsolete and will not work.

There are also some tests added in t/pg_problems that test that rendering with the WeBWorK::PG module works and demonstrate how to use it in unit tests.

There was also a "memory bleed" issue that was found with the Applet.pm module and the AppletObjects.pl macro caused by extending the Applet package in the macro. Instead of extending the Applet package, a derived package (PGApplet) is created and used in the macro. The GeogebraWebApplet package needs to derive from the PGApplet package, and so it needs to be moved to the macro. Also, the Applet.pm package no longer derives from the PGcore package. It doesn't need to as it uses none of the PGcore module methods. A similar issue occurs with the tableau.pl macro, but that was not fixed. So see the issue here, add a for my $i (0 .. 1) loop around the code inside the subtest in t/macros/load_macros.t. When the tableau.pl macro is loaded the second time, a warning will be issued.

@drgrice1
Copy link
Member Author

Note that the added tests use the Test2::Suite (via the Test2::V0 bundle). So pull request #702 is needed to regain uniformity for the tests.

@drgrice1 drgrice1 changed the title Move the WeBWorK::PG module to PG and improve PG's local configuration.j Move the WeBWorK::PG module to PG and improve PG's local configuration. Aug 20, 2022
@pstaabp
Copy link
Member

pstaabp commented Aug 21, 2022

I'm starting to dig into this. First of all, it looks like we'll need to update the database tables in the admin course.

Overall, things look great. I have some problem sets for testing over the past year or so and old Flash Applets give the following error: Can't locate object method "insertAll" via package "Applet" at line 85 of (eval 5768) instead of saying that they are no longer supported.

@drgrice1
Copy link
Member Author

Yes, to test this you will need to upgrade database tables.

This flash applets error is fixed. You will need to reset the branch (or delete it and check it out again) to get the changes.

@pstaabp
Copy link
Member

pstaabp commented Aug 21, 2022

You will need to reset the branch (or delete it and check it out again) to get the changes.

Odd. Still seeing this and have tried deleting and clearly back on both of these branches.

@pstaabp
Copy link
Member

pstaabp commented Aug 21, 2022

On the chromatic problems, On the latter two you listed above, I'm getting the following errors:

Use of uninitialized value in numeric gt (>) at [PG]/lib/Chromatic.pm line 275
Use of uninitialized value in numeric eq (==) at [PG]/lib/Chromatic.pm line 275

@drgrice1
Copy link
Member Author

drgrice1 commented Aug 21, 2022

Make sure you reset the pg pull request to fix the flash issue, not this one.

Edit: Oops, this is the pg pull request.

@drgrice1
Copy link
Member Author

I am not seeing the chromatic errors.

@drgrice1
Copy link
Member Author

What seed do you have for the chromatic problems? Maybe it is seed specific.

@drgrice1
Copy link
Member Author

Okay, I didn't actually commit the changes to AppletObjects.pl. Now they are in. Sorry.

I changed seeds, and now I am seeing the use of uninitialized values for the last chromatic problem. I will look into it.

@pstaabp
Copy link
Member

pstaabp commented Aug 21, 2022

Applets is fixed now. :)

@pstaabp
Copy link
Member

pstaabp commented Aug 21, 2022

Also, wanted to note, that in the PG_ROOT directory, prove -r t runs everything successfully. There's a number of test problems in the t directory that should either be removed/moved or have tests that run them. (not for this PR though.)

@drgrice1
Copy link
Member Author

The chromatic errors are fixed. It was a faulty initialization.

@drgrice1
Copy link
Member Author

Yeah, the t directory needs to be cleaned up. TODO

@drgrice1 drgrice1 force-pushed the pg-envir-work branch 3 times, most recently from 6ac2100 to 5884136 Compare August 21, 2022 16:37
Copy link
Member

@drdrew42 drdrew42 left a comment

Choose a reason for hiding this comment

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

Mostly inconsequential feedback from initial read-through. Testing forthcoming.

@drgrice1 drgrice1 force-pushed the pg-envir-work branch 2 times, most recently from fb5fe42 to f2e0ed8 Compare August 28, 2022 20:10
Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

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

After more testing, all of the chromatic tests work again. Overall, things look good.

@drgrice1 drgrice1 force-pushed the pg-envir-work branch 4 times, most recently from e1f076e to 59a0478 Compare September 8, 2022 12:45
@drgrice1 drgrice1 force-pushed the pg-envir-work branch 6 times, most recently from 81b9834 to 92f7883 Compare September 19, 2022 11:47
@drgrice1
Copy link
Member Author

This now includes the improved warning handling that we discussed.

@drgrice1 drgrice1 closed this Sep 22, 2022
@drgrice1 drgrice1 reopened this Sep 22, 2022
@drgrice1
Copy link
Member Author

Uhh, did not mean to close with comment.

Remove the ALWAYS_SHOW_HINT_PERMISSION_LEVEL and
ALWAYS_SHOW_SOLUTION_PERMISSION_LEVEL settings.  Whether hints and
solutions are shown is now determined by the front end.

The pg flag/variable $showHint has been removed.  The front end has
complete control on when hints are shown.  Not the problem author.

Also the options to use knowls for solutions and hints have been
removed.  Always use knowls for solutions and hints.

Showing the various pieces of debugging information for a problem is now
controlled with translation options.  The front end is responsible for
converting permissions into these debugging options.  The
VIEW_PROBLEM_DEBUGGING_INFO special pg environment variable was removed
and is one of these debugging options.

Many unnecessary PG environment variables have been removed.  One of
these is the session key.

Remove reduced scoring entirely from PG.  That is the responsibility of
the front end to implement.

Pg now has not dates and and no permissions.

The color.c file has been removed.  The method in that file has been
reimplemented in Chromatic.pm using perl.  It is perhaps a bit slower,
but not significantly in the scope of rendering a problem.  This can be
tested with the problems
Library/NAU/setGraphTheory/FindChromaticNumber.pg,
Library/NAU/setGraphTheory/FindRoomColor.pg, and
Library/NAU/setGraphTheory/RoomSchedule.pg
which happen to be all known problems that use the Chromatic module.

The image generator has been rewritten to remove the dvipng_align hack
and to allow the usage of sqlite for a database backend for storing
dvipng depths.

Note that PGalias no longer uses the course name, user login, set
number, and problem number for generating resource UUID's.  It now only
uses the problem seed, psvn, and problemUUID.  It is the caller's
responsibility of ensuring that problemUUID guarantees the necessary
uniqueness.
The t/macros/load_macros.t file tests that all macros load without
errors or warnings.

There is one macro that will not work in this test and it is skipped.
That is the answerDiscussion.pl macro.  It requires a file that is not
part of the PG distribution.  It also has invalid `defined(@array)`
calls.

Warnings found in other macro files were fixed.

The PG_module_list.pl macro was deleted.  That is clearly obsolete and
will not work.

There are also some tests added in t/pg_problems that test that
rendering with the WeBWorK::PG module works and demonstrate how to use
it in unit tests.
AppletObjects.pl macro caused by extending the Applet package in the
macro.  Instead of extending the Applet package, a derived package
(PGApplet) is created and used in the macro.  The GeogebraWebApplet
package needs to derive from the PGApplet package, and so it needs to be
moved to the macro.  Also, the Applet.pm package no longer derives from
the PGcore package.  It doesn't need to as it uses none of the PGcore
module methods.
The server really should not be checking urls in this way.  If an
invalid url is given, let the error come out in the browser.  This is
the correct way to deal with this.
Many subroutines are redefined in macros.

In addition if multiple WeBWorK::PG::Translator instances are created in
the same process, the safe compartment is not entirely isolated between
instances, and this causes redefine warnings if a macro is loaded in
both instances.  This is due to a memory leak somewhere and needs to be
fixed.
back to the original file in which the error occurred.
@pstaabp pstaabp merged commit df59e8f into openwebwork:develop Oct 12, 2022
@drgrice1 drgrice1 deleted the pg-envir-work branch October 12, 2022 19:37
drgrice1 added a commit to drgrice1/pg that referenced this pull request Jan 27, 2023
drgrice1 added a commit to drgrice1/pg that referenced this pull request Jan 30, 2023
drdrew42 added a commit to openwebwork/renderer that referenced this pull request May 22, 2023
Update the renderer for the restructuring of PG in openwebwork/pg#709
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