-
-
Notifications
You must be signed in to change notification settings - Fork 368
CMake: Create grass.bat and add scripts to PATH #6383
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
base: main
Are you sure you want to change the base?
Conversation
Would prepending GRASS_PYDIR to sys.path: Line 2098 in 92dc5b8
be an alternative to versioning the file name? |
Hm, regardless, we should prepend. I'll adjust my PR. We could also import in more low level way to avoid the version. |
No, it doesn't work. Still trying to self-import because the main script name is the same as the package name. |
It's OK on UN*X because the main script is |
Oh, too bad. Not sure what to do, it is not ideal to re-add the version to the main executive. |
This PR doesn't change anything for UN*X. |
I understand it is only for Win. If you will do scripting on Win, wouldn't |
For Python scripting, yes |
If I understand correctly, the situation now is:
This looks platform-appropriate to me. Anyway, I made the change with prepend in #6393. In my local test on Linux, it seems that
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be for merging the versioned part of this PR right now. If we want to make it better in the future, we can refine #6393, use importlib, or the __main__.py
file Python mechanics.
The scripts part is not clear to me. With Autotools we don't have separate scripts on Linux, but we may have them for Windows. I don't know what is the exact reasoning behind that. It is clear that Linux just treats all the executables the same, while more care is needed to run a Python script on Windows.
else: | ||
# Without FHS, scripts are separated like in the source code. | ||
path = os.path.join(install_path, "scripts") | ||
if os.path.exists(path): | ||
paths.appendleft(path) | ||
|
||
# Without FHS, scripts are separated like in the source code. | ||
path = os.path.join(install_path, "scripts") | ||
if os.path.exists(path): | ||
paths.appendleft(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confusing, I would say the original code was wrong for Windows with Autotools because it would skip it. Both the old code and new code should be fine for Linux because there should be no scripts directory AFAIK, but then the comment is misleading. I didn't look at blame - if I'm the author, I would be twice as confused as I'm now. :-)
In any case, testing os.path.exists(path)
is good, but skipping based on the platform is even better if we already have that info.
# START_UP is the variable used in grass.py, grass.sh.in and grass.bat.in | ||
set(START_UP ${PROJECT_NAME_LOWER}) | ||
if(WIN32) | ||
set(START_UP "${START_UP}.py") | ||
set(START_UP "${START_UP}${GRASS_VERSION_NUMBER}.py") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would appreciate a comment in the source code providing the reason. Something like "Python file has version to avoid issues with import grass statements". It could also be called grass_main.py
instead of using the version if that would simplify things or help clarity.
I made even further changes to #6393 and it works for me on Linux with grass.py. I would give it a try before merging the version piece here. We may still want to avoid the failed import with the "versioned" file, but we don't have to, because that's handled gracefully with #6393. #6393 needs a review. |
This PR
grass.py
tograss<VERSION>.py
to avoidgrass.bat
fromlib/init/grass.bat.in
, andscripts
toPATH
for batch files.