ucphhpc / migrid-sync

MiGrid workspace where master branch is kept strictly in sync with SF upstream svn repo. Any development or experiments should use a branch. You probably want to fork your own clone or work e.g. on the edge branch if you wish to contribute.
GNU General Public License v2.0
3 stars 3 forks source link

Do the minimum required to allow executing safeinput to work on Py3. #58

Closed albu-diku closed 1 month ago

albu-diku commented 1 month ago

Backport the switch from cgi.escape to html.escape so mig.shared.safeinput can be imported across versions. Wrap the existing library level main() code in a transitional test thereby confirming basic working.

Note that main() must still be converted into individual test cases.

jonasbardino commented 1 month ago

Hmm, I'm not really sure why you want to mangle sys.path in these - that should already be handled by PYTHONPATH.

Unconditionally importing html may break python2, so that's probably a no-go for edge. At least it looks that way on my minimal local python2 installation:

0|~ > python2
Python 2.7.18 (default, Oct 15 2023, 16:43:11) 
[GCC 11.4.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import html
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named html

According to the python docs at https://docs.python.org/3/library/html.html#html.escape the html.escape was added in 3.2. We can of course use a try import escape from html construct with fallback to importing the cgi one.

Anyway, I approved and ran the checks and they passed as you can see.

albu-diku commented 1 month ago

@jonasbardino there is a chance that my lack of complete understanding of conventions has lead to at least some of your points here - response below very much in light of that.

Re PYTONPATH - I added that because attempting to run this thing directly (for its main method) did not work and gave import errors. But thinking about this some: when run under the test suite we will have arranged sys.path to include the libraries (via support.py) but missing in my mental model must be that we separately arrange a suitable path at, I guess, the level of the server process? Want to confirm this because it will explain some of what I've hit with import errors but certainly cause a slight change in my approach in adjusting this.

Given the above, I'm about to push a change that removes sys.path fiddling from the file itself and instead adds a transitionary test file that imports mig.shared.safeimport and executes its main instead. Rationale for doing so is twofold: first, it ensures the file is importable in both python versions and second, it's a way of piecemeal getting value from the runnable code in the lib file.

albu-diku commented 1 month ago

Follow-up: also confirmed here now that your Python 2 test was not entirely valid because you tested it without the dependencies installed :)

You can invoke a 'populated' version as follows: ./envhelp/python2 ... ..and when doing so importing html works fine.

jonasbardino commented 1 month ago

Yeah, I think we may have to go over the import model that we implicitly assume and figure out how it fits the new tests setup. Some of the absolute imports and relying on PYTHONPATH goes back to the porting effort to support both py2 and py3 with the 'future' module and the futurizeall.py helper.

jonasbardino commented 1 month ago

Right, I'm aware that my local python2 installation from Ubuntu 22.04 is very limited (almost crippled) and doesn't even have pip support. Yet, we need to identify the exact dependencies and make sure they are always available on all sites before adding any such potentially breaking code to edge, as it is actively used in production without that new requirements.txt.

albu-diku commented 1 month ago

Replaced by https://github.com/ucphhpc/migrid-sync/pull/61.