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

Provide mlocate for running unit tests in container #383

Closed
wants to merge 1 commit into from

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented May 23, 2023

I think we'd better have this installed pre-hand, so users can run unit tests if they want even on the remote deployment, otherwise has to install it manually as root.

@danielhollas
Copy link
Contributor

This is indeed very annoying, but I would actually suggest to go the other way and try to get rid of the mlocate dependency in tests. Do we know exactly where it comes from? Can we get rid of it? mlocate is I think something that I'd like to avoid in a production container, as it is a service that we shouldn't need.

If we can't get rid of it from tests, maybe we could just have a dirty mlocate script that would produce the expected output?

@unkcpz
Copy link
Member Author

unkcpz commented May 23, 2023

Here is where the locate is needed, from the message posted by @sphuber in slack:

You can see here in pgtest https://github.com/jamesnunn/pgtest/blob/417f0d89e66a9395db694a96122e0d5ac0097d59/pgtest/pgtest.py#L117 how it is trying to use locate to find the pg_ctl command. Not sure why it is using a custom implementation for which instead of shutil.which. Might be for backwards compatibility. You could try running pgtest.which directly and see if that works and try shutil.which. If the former fails and latter works, maybe there is a problem with the pgtest implementation

Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Thanks, that was helpful. I looked at the code, and the thing is the the locate command is used by pgtest only as a fallback when other things fail. What the underlying problem is that we install psql in a separate conda environment aiida-core-services and so the pg_ctl command is not in PATH. However, I just tested that when I activate this environment with conda activate aiida-core-services I can run the tests. @unkcpz can you give it a try.

If that works for you than honestly I would not do anything here and just accept that you need to activate the environment. Alternatively, we could perhaps make a symlink of pg_ctl to some folder that's in PATH or some other hack, but thb that's just asking for problems in the future...

@unkcpz
Copy link
Member Author

unkcpz commented May 24, 2023

However, I just tested that when I activate this environment with conda activate aiida-core-services I can run the tests. @unkcpz can you give it a try.

That works! Thanks!

I think we need somewhere for the developers of the app to know about this.

@unkcpz
Copy link
Member Author

unkcpz commented May 24, 2023

I put the instruction in the readme of qeapp repo for the moment, aiidalab/aiidalab-qe#403

@unkcpz unkcpz closed this May 24, 2023
@danielhollas
Copy link
Contributor

Cool, I've also opened an issue on the pgtest package to better handle this case

jamesnunn/pgtest#25

@unkcpz unkcpz deleted the fix/xx/add-mlocate branch December 15, 2023 20:01
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.

None yet

2 participants