Skip to content

Feature: Add $MDSPLUS_DIR/python to PYTHONPATH in etc/envsyms - #3074

Open
WhoBrokeTheBuild wants to merge 2 commits into
MDSplus:alphafrom
WhoBrokeTheBuild:envsyms-pythonpath
Open

Feature: Add $MDSPLUS_DIR/python to PYTHONPATH in etc/envsyms#3074
WhoBrokeTheBuild wants to merge 2 commits into
MDSplus:alphafrom
WhoBrokeTheBuild:envsyms-pythonpath

Conversation

@WhoBrokeTheBuild

@WhoBrokeTheBuild WhoBrokeTheBuild commented Jul 16, 2026

Copy link
Copy Markdown
Member

This removes the long-broken MDSPLUS_SETUP_PY and instead adds $MDSPLUS_DIR/python to the PYTHONPATH in $MDSPLUS_DIR/etc/envsyms.
This should allow users to import MDSplus in python after an install with no additional setup

Also add import MDSplus steps to the installer tests

@WhoBrokeTheBuild WhoBrokeTheBuild self-assigned this Jul 16, 2026
@WhoBrokeTheBuild WhoBrokeTheBuild added api/python Relates to the Python API os/linux This is present on or relates to Linux os/mac This is present on or relates to Mac OSX labels Jul 16, 2026
This removes the long-broken `MDSPLUS_SETUP_PY` and instead adds `$MDSPLUS_DIR/python` to the `PYTHONPATH` in `$MDSPLUS_DIR/etc/envsyms`.
This should allow users to `import MDSplus` in python after an install with no additional setup
Since MDSplus is now on the PYTHONPATH by default, we're able to test importing during the install tests

@mwinkel-dev mwinkel-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simple change. Looks good. Approving this because I'm assuming Jenkins will rebuild it.

Note though that one of the "Test Packages" tests failed because it returned exit status 42.

@dgarnier dgarnier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree this is consistent with the current use of PYTHONPATH and MDSplus. However, I'm not a huge fan of this whole mechanism and at OpenStar we are doing something entirely different and explicitly not relying on PYTHONPATH.. preferring to use venv based solutions (see my request to have MDSplus build its own wheel during the install instead).

This change would effectively just have me modify our environment to take it back out... so I'm not a fan of this change.

@WhoBrokeTheBuild

Copy link
Copy Markdown
Member Author

For what it's worth Darren I agree, there's a reason I haven't implemented this in all the years we've been fighting with the python package install. You can see the discussions in the discord that lead to this PR being submitted.

I'm also not a fan of the wheel solution because it (as I understand it) places a copy of the python files in the venv rather than pointing to the $MDSPLUS_DIR/python folder, which makes sense for most packages but I don't like it for ours.

Then there's the additional issues of trying to modify the default python environment on install, and while users can absolutely setup a venv I would really like import MDSplus to work on a fresh install.

With some effort, I could probably get MDSPLUS_SETUP_PY to trigger adding PYTHONPATH=$MDSPLUS_DIR/python to etc/envsyms if people would refer that? It means that this will only affect the users that want it, which is good, but then it also won't affect users who don't know about the rather obscure variable.

@dgarnier

Copy link
Copy Markdown
Contributor

I'm also not a fan of the wheel solution because it (as I understand it) places a copy of the python files in the venv rather than pointing to the $MDSPLUS_DIR/python folder, which makes sense for most packages but I don't like it for ours.

Oh. That gives me an idea. Let me see if it works. I think a wheel can be made that does just what you want.. just installs a .pth that points to $MDSPLUS_DIR/python (or the absolute install directory which is probably actually what we want. I'll send a PR if it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api/python Relates to the Python API os/linux This is present on or relates to Linux os/mac This is present on or relates to Mac OSX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants