Skip to content
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

fix qtype_stack phpunit maxima depedency #857

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GGeorggg
Copy link

fixes: #729

@aharjula
Copy link
Member

This worries me a bit, I doubt having a sudo call here (or anywhere) is a good idea. The original issues unit test workflow issue is something that should probably be fixed at the Travis config level. I doubt this so much that I won't approve the workflow checks, but I asked Tim to check, and maybe he wishes to see what exactly happens if the workflow starts calling sudo, should be safe for that particular command but still...

Thank you anyway.

@GGeorggg
Copy link
Author

Hi @aharjula

you are right - this is not the right way to fix this, but i could not yet figure out how moodles CI/CD workflow works and there is an year old issue, which could be fixed by that - so just in case someone else gets his/hers CI/CD Pipeline stuck after adding qtype_stack i would go for this ugly fix than breaking the pipeline

regards
Georg

@aharjula
Copy link
Member

Well, if I understood correctly, you (or the original issues people) are trying to use Moodles core CI/CD scripts (of some kind from somewhere) to test a Moodle install that has additional plugins, and in this case, plugins that have additional requirements, even requirements that go beyond the PHP extensions/libraries that Moodle itself tracks. Moodle has no internal mechanism for configuring requirements as complex as Maxima is for STACK (as far as I know). After all, we would ideally not have Maxima on the actual Moodle server at all, and in many cases, people want to install something other than the default Ubuntu GCL Maxima (the primary reason being better Unicode support).

Those scripts do already do some installs themselves, especially those PHP extensions, and the correct way would be to tune those installs to include STACKs requirements if STACK is present in your configuration. You would also want them to match all the other libraries your production configuration has or does not have at the versions you use and not use whatever versions Moodles default tests use. If I understand your (or the original issues) objective correctly, the place where the "fix" should happen is your custom CI/CD config, that type of thing cannot be pushed up to the original individual components as it would push your config to other people in a way that would require trickery to override.

Now why this particular solution is something that I don't want to see is due to the following use case:

  1. I have an Ubuntu-based virtual machine where I manually run STACK unit tests and have been running them for the last decade.
  2. It is not a CI config it is as close to an actual install as possible but with PHPUnit.
  3. Now, if I do a version upgrade and run the PHPUnit init script on that with the intention of testing automatic configuration and your modification is in play, what happens is:
    • The init script hangs and starts asking for a password. And I have no idea why. It sure stops my semi-automated processes.
    • To stop it from asking for that password, I would need to allow the test running user to call sudo for that particular install command without authentication. Which is not something I would like to do. Alternatively, I could remove sudo from the system to ensure that that call fails fast but doing that in Ubuntu might not be the best idea. I can also smash wrong passwords into that prompt and make it fail that way, but that could cause other types of side effects...
    • Now, if I allow it to install that particular maxima I end up in a situation where I cannot test autoconfiguration of alternative maxima installations as that packaged one will be the primary one. Also, if those switches in the apt-get command lead to the default maxima package silently replacing my custom package, through some conflict resolution, things get even more complicated.

Basically, while you might "fix" some CI external to STACK, you would break traditional/manual unit tests for STACK on Ubuntu by including a new forced step that could cause trouble. If anything like this would end up in the code base, I would really hope that it would be, by default, disabled and would require something like a particular environment variable or config-option to be enabled. If not disabled by default, it should at least have options for disabling it independent of the current conditions it is being tied to.

Having gone through the CI workflows STACK has in github, this would probably not break them (they already seem to call sudo and do seem to use the default maxima), but it could cause limitations for their development if we were to start testing varying Maximas over there.

In any case, thank you for documenting this for others that may have chosen this path for setting up their whole Moodle CI/CD processes.

@GGeorggg
Copy link
Author

GGeorggg commented Oct 3, 2022

Hi @aharjula

not sure if this is a nicer way to work around the issue, but it will for sure not break your manual phpunit test - this will check for CI Variable to be set:

https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables

regards
Georg

@aharjula
Copy link
Member

aharjula commented Oct 3, 2022

Indeed it will protect my own manual tests, and that is nice. However, it still makes it difficult for STACKs own CI logic to use non-standard Ubuntu Maxima combined with auto-configuration, we are not doing that now, but this will affect any future developments in that direction. Also, with STACKs own CI scripts, this would trigger reinstallation of the Maxima that was already installed, not a big thing but wasted cycles anyway.

I still think this whole issue should be solved at the top-level CI config of that Moodle to which you have added STACK. And the solution should focus more on actually matching the versions in use in production. There must be a way to add things to that Moodle CI/CD script execution environment.

In any case, someone else decides where this will go. To me, it still is the wrong place to solve this, but someone else may think that this is a simple thing we can do to help someone and consider the possible trouble this might cause small enough.

@GGeorggg
Copy link
Author

GGeorggg commented Oct 4, 2022

Hi @aharjula

  • we could additionally check for ${GITHUB_REPOSITORY} against maths/moodle-qtype_stack

or

  • check if maxima is available in path, already

this would solve the two issues about wasting cycles and would allow to use custom maxima version, i agree with this not being a nice fix but rather a quirk.

I agree that there should be a moodle ci/cd way of resolving dependencies for plugins, but that is not available right now.

@GGeorggg
Copy link
Author

GGeorggg commented Oct 6, 2022

It seems like moodle-tool_imageoptimize has similar external depedencies - the have a check_package_command_for_testing function:

https://github.com/tigusigalpa/moodle-tool_imageoptimize/blob/2774bab2850fb19288780b593aebb8211efc0f71/classes/tool_image_optimize_helper.php#L547-L554

Handling of missing external dependencies is done here:

https://github.com/tigusigalpa/moodle-tool_imageoptimize/blob/master/tests/imageoptimize_task_test.php#L53-L63

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.

Errors running GitHub actions workflow PHP unit tests
2 participants