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

GRASS GIS 8.0 changes #1597

Merged
merged 27 commits into from
May 31, 2021
Merged

GRASS GIS 8.0 changes #1597

merged 27 commits into from
May 31, 2021

Conversation

neteler
Copy link
Member

@neteler neteler commented May 23, 2021

This PR is meant for testing (do not merge into master yet!) as it contains needed changes for a GRASS GIS 8.0 .0alpha release.
It may be used for local testing by applying this PR to the latest GRASS GIS-core in the master branch.

Update 28 May 2021:
This PR may become the basis for the changes in the master branch to GRASS GIS 8.0.
A 8.0.0alpha release branch may be forked after merge of this PR.

Please verify thoroughly.

(note that some CI tests will fail as they expect grass79 and not grass80 as the name of startup script; in addition, there is no grass8 branch yet in addons)

To locally apply and try it:

  1. checkout GRASS GIS master from GitHub, cd into it
  2. if you already have it, make distclean
  3. wget https://github.com/OSGeo/grass/pull/1597.diff
  4. patch -p1 < 1597.diff
  5. configure ... ; make
  6. start it from bin.x86_64-pc-linux-gnu/grass80 (the name of architecture in the path may differ)

NEWS Outdated Show resolved Hide resolved
REQUIREMENTS.html Outdated Show resolved Hide resolved
db/db.login/db.login.html Outdated Show resolved Hide resolved
doc/grass_database.html Outdated Show resolved Hide resolved
Copy link
Member

@hellik hellik left a comment

Choose a reason for hiding this comment

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

left mostly windows related comments

doc/grass_database.html Outdated Show resolved Hide resolved
gui/wxpython/docs/wxGUI.html Outdated Show resolved Hide resolved
@@ -18,7 +18,7 @@ <h2>DESCRIPTION</h2>
Toolboxes are configured through two XML files (<tt>main_menu.xml</tt> and
<tt>toolboxes.xml</tt>) located in your user home
GRASS directory, subdirectory <tt>toolboxes</tt>
(<tt>$HOME/.grass7/toolboxes/</tt> on UNIX).
(<tt>$HOME/.grass8/toolboxes/</tt> on UNIX).
Copy link
Member

Choose a reason for hiding this comment

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

adding a windows example?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, pls donate an example

Files should be placed in user home directory in ``.grass7/toolboxes``
subdirectory, e.g. ``/home/john/.grass7/toolboxes``.
Files should be placed in user home directory in ``.grass8/toolboxes``
subdirectory, e.g. ``/home/john/.grass8/toolboxes``.
Copy link
Member

Choose a reason for hiding this comment

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

adding a windows example?

lib/init/variables.html Outdated Show resolved Hide resolved
lib/init/variables.html Outdated Show resolved Hide resolved
# uncomment when using standalone WinGRASS installer
# grass7bin = r'C:\Program Files (x86)\GRASS GIS 7.9.0\grass79.bat'
# grass8bin = r'C:\Program Files (x86)\GRASS GIS 8.0.0\grass80.bat'
Copy link
Member

Choose a reason for hiding this comment

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

do we provide still a 32 bit standalone installer C:\Program Files (x86)? @landam

@wenzeslaus
Copy link
Member

To locally apply and try it:
...
3. wget https://github.com/OSGeo/grass/pull/1597.diff
4. patch -p1 < 1597.diff

Or gh pr checkout 1597 following the Open with button right from the PR title. (Or manually, add the neteler fork repo as another remote to your local repo and get the grass_gis_800_alpha from there.)

Comment on lines 224 to 230
<div class="code"><pre>
grass79 --text ~/grassdata/mylocation/mymapset
# Linux, Mac, *BSD, ...:
grass80 --text ~/grassdata/nc_spm_08_grass7/user1

# Windows
grass80 --text D:\grassdata\nc_spm_08_grass7\user1
</pre></div>
Copy link
Member

Choose a reason for hiding this comment

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

Do you think that someone who has an issue figuring out the need for a Windows path from the unix example (as before) will parse through the # comments in the example? Either delete the Windows example or make it a separate paragraph.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean? In my experience, students want to see it written, they do not want to figure it out themselves...

Copy link
Member

Choose a reason for hiding this comment

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

My question is how large is the overlap between the groups: Group 1 can replace unix path by Windows path easily. Group 2 is not distracted or confused by the # comments. I'm guessing pretty small. In other words, if the reader is familiar with unix/Bash syntax, the Windows piece is not needed. And if a unix path is a stumbling block, so is a Bash comment.

Copy link
Member

Choose a reason for hiding this comment

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

in my experience, many manual examples are too *nix biased and exclude average windows user by not giving literal working windows examples. I agree here with @veroandreo.

Copy link
Member

Choose a reason for hiding this comment

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

...many manual examples are too *nix biased...

I think what would help is saying that it is a Bash or whatever example so that it would be clear that commands or syntax may be different (on Windows) and that it is not "standard GRASS" but rather "standard Bash/unix/...".

Otherwise, I think we need Python examples in manual pages more than Windows examples anyway.

...exclude average windows user...

Similarly to the "dropping terminal" PR, I continue being puzzled about who are these average Windows users attached to their command lines. Isn't that a stereotype about Linux users, not Windows ones? :-)

INSTALL Outdated
and the scripts (grass79, ...) in
and the scripts (grass80, ...) in
Copy link
Member

Choose a reason for hiding this comment

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

What about dropping the 80 part at least from the documentation? On a reasonable system, it should install as grass anyway. I also noticed I usually write ls and not ls830, gdalinfo, not gdalinfo32... Seems like less work for the releases, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally fine for me but then we may also need to actually generate a grass script at compile time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, fine for me.

This PR is meant for testing (do not merge into master!) as it contains needed changes for a GRASS GIS 8.0.0alpha release.

I don't understand where this will go.

As I understand there are still different opinions (https://lists.osgeo.org/pipermail/grass-dev/2021-May/095136.html and related) on when to create a branch.

If it will be released, it should go to the main repo. A branch would make sense to me.

We could asap, as I don't see much point in keeping "7.9" forever:

  • merge this PR into the main branch (changing VERSION from "8.0.0alpha" to "8.0.dev")
  • then create the "releasebranch_8_0" branch off the main branch (changing VERSION therein to "8.0.0alpha")
  • cherry-pick future relevant changes from main branch to "releasebranch_8_0" as we have done with other release branches

Copy link
Member

Choose a reason for hiding this comment

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

Generally fine for me but then we may also need to actually generate a grass script at compile time.

A version independend startup script is nice if this does not interfere with installing different GRASS versions in parallel.
So we might need to keep grass78 and grass80 anyway? Just to make sure different version in parallel are considered...

INSTALL Outdated Show resolved Hide resolved
REQUIREMENTS.html Outdated Show resolved Hide resolved
<li> in the 'home' directory, i.e. <tt>$HOME/.grass7/dblogin</tt> (Unix-like systems)</li>
<li> <tt>%APPDATA%/GRASS7/dblogin</tt> (MS-Windows)</li>
<li> in the 'home' directory, i.e. <tt>$HOME/.grass8/dblogin</tt> (Unix-like systems)</li>
<li> <tt>%APPDATA%\Roaming\GRASS8\dblogin</tt> (MS-Windows)</li>
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't / just work on Windows these days, @hellik?

Copy link
Member

@HuidaeCho HuidaeCho May 24, 2021

Choose a reason for hiding this comment

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

/ sort of works on Windows (https://answers.microsoft.com/en-us/windows/forum/windows_7-files/when-did-windows-start-accepting-forward-slash-as/aa033741-0fc6-43e3-ab6e-165020794126), but I don't think it's a good idea to use / in general (https://bytes.com/topic/python/answers/23123-when-did-windows-start-accepting-forward-slash-path-separator), not in this specific case.

cd /users # works fine
mkdir \data
cd /data  # doesn't work because /d is an option

Copy link
Member

Choose a reason for hiding this comment

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

cd /data # doesn't work because /d is an option

AFAIK it's

cd data

cd \data changes here to an existing directory in the toplevel d: , altough I've started e.g. in D:\temp\tm18> where the data subfolder exists.

D:\temp\tm18>cd /data
Das System kann den angegebenen Pfad nicht finden.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't / just work on Windows these days, @hellik?

the interesting thing is - and almost nobody knows it ;-) - it worked since the beginning of MS windows

D:\>cd D:\temp\tm18\data
D:\temp\tm18\data>
D:\>cd D:/temp/tm18/data
D:\temp\tm18\data>

though the MS windows standard, everywhere promoted, is:

D:\temp\tm18\data

and you can't be sure that it works in all circumstances of myriads of different varieties of batch files, python scripts, R scripts in windows .....

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there are any circumstances where it is still an issue except for that /d issue. Python scripts should never have explicit slash or backslash in them. The best practices for R I have seen argued for forward slashes on all platforms (saying to make them work on macOS and Linux, so they actually seemed written from Windows point of view).

@wenzeslaus
Copy link
Member

This PR is meant for testing (do not merge into master!) as it contains needed changes for a GRASS GIS 8.0.0alpha release.

I don't understand where this will go. If it will be released, it should go to the main repo. A branch would make sense to me.

@neteler
Copy link
Member Author

neteler commented May 26, 2021

It seems like you can re-conceptualize this PR to be a 7-to-8 transition, merge it to master branch, and do the alpha release changes elsewhere. Thoughts?

Meanwhile I also believe that this PR is the preparation of the master branch for G8. So.

  • a 8.0.0alpha branch shall just be a "throw away/ignore later" branch, created after merging of this PR
  • the real release_branch_8_0 can then be created at a later stage, after gaining some experience with the "throw away" 8.0.0alpha branch.

@neteler
Copy link
Member Author

neteler commented May 26, 2021

ModuleNotFoundError: No module named 'grass.grassdb'; 'grass' is not a package

My guess: The directory with grass.py is on Python (import) path (sys.path). Likely because it is the current directory. Python picks grass.py instead of the grass package. Startup ends in flames.

I see. Shall I revert the parts concerning Windows at this point? I happily accept suggestions how to deal with this.

@neteler
Copy link
Member Author

neteler commented May 26, 2021

ModuleNotFoundError: No module named 'grass.grassdb'; 'grass' is not a package

My guess: The directory with grass.py is on Python (import) path (sys.path). Likely because it is the current directory. Python picks grass.py instead of the grass package. Startup ends in flames.

Does anyone have an idea how to deal with this collision? It's the remaining showstopper to merge this PR.

@neteler neteler changed the title WIP: GRASS GIS 8.0.0alpha changes WIP: GRASS GIS 8.0.0 changes May 28, 2021
@neteler
Copy link
Member Author

neteler commented May 28, 2021

Shall I revert the parts concerning Windows at this point?

Ok, I have reverted all Windows related changes for now.
The only Windows related change is "grass79" -> "grass80" (ignoring any documentation related issues which may be addressed later/in a separate PR).

@landam
Copy link
Member

landam commented May 28, 2021

The only Windows related change is "grass79" -> "grass80" (ignoring any documentation related issues which may be addressed later/in a separate PR).

The Windows issues can be solved in separated PR. Lets move on with this important PR.

@neteler neteler changed the title WIP: GRASS GIS 8.0.0 changes WIP: GRASS GIS 8.0 changes May 29, 2021
@neteler neteler changed the title WIP: GRASS GIS 8.0 changes GRASS GIS 8.0 changes May 29, 2021
@neteler
Copy link
Member Author

neteler commented May 29, 2021

The Windows issues can be solved in separated PR. Lets move on with this important PR.

With the CI hack in ea31d27 also OSGeo4W builds again. The CI only fails because of the not (yet) existing grass8 branch in the addons repo.

I suggest to merge this PR.

@petrasovaa petrasovaa removed their request for review May 30, 2021 03:48
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Shouldn't the following lines:

INSTDIR='${prefix}'"/grass${GRASS_VERSION_MAJOR}${GRASS_VERSION_MINOR}"

grass/configure

Line 1476 in e5379bb

INSTDIR='${prefix}'"/grass${GRASS_VERSION_MAJOR}${GRASS_VERSION_MINOR}"

be changed to:
INSTDIR='${prefix}'"/grass"
?

Or is this needed for Windows build?

Else, fine for me.

@neteler
Copy link
Member Author

neteler commented May 31, 2021

Shouldn't the following lines:
...
be changed to:
INSTDIR='${prefix}'"/grass"
?

Or is this needed for Windows build?

I'd keep it for now and move on.

@nilason
Copy link
Contributor

nilason commented May 31, 2021

Shouldn't the following lines:
...
be changed to:
INSTDIR='${prefix}'"/grass"
?
Or is this needed for Windows build?

I'd keep it for now and move on.

I needed that change on Mac at least, compilation (or was it install?) failed without it. But that can be fixed with other issues which will likely pop up after merge.

@neteler neteler requested a review from hellik May 31, 2021 18:21
@nilason
Copy link
Contributor

nilason commented May 31, 2021

Shouldn't the following lines:
...
be changed to:
INSTDIR='${prefix}'"/grass"
?
Or is this needed for Windows build?

I'd keep it for now and move on.

I needed that change on Mac at least, compilation (or was it install?) failed without it. But that can be fixed with other issues which will likely pop up after merge.

I now tested again, working fine. Please ignore my previous comment.

@neteler neteler merged commit ae610b8 into OSGeo:master May 31, 2021
@neteler neteler deleted the grass_gis_800_alpha branch May 31, 2021 19:12
neteler added a commit to OSGeo/grass-addons that referenced this pull request May 31, 2021
@HuidaeCho
Copy link
Member

Dropping the version from the startup script broke my cross-compilation cron jobs. Have we decided to not use ${MAJOR}${MINOR}? What was wrong with it?

@neteler
Copy link
Member Author

neteler commented Jun 17, 2021

Dropping the version from the startup script broke my cross-compilation cron jobs. Have we decided to not use ${MAJOR}${MINOR}? What was wrong with it?

It has been kept for Windows, and dropped elsewhere. Maybe @wenzeslaus and @nilason can comment here?

@HuidaeCho
Copy link
Member

It has been kept for Windows, and dropped elsewhere.

And why is this inconsistency? Quoting @ninsbl

A version independend startup script is nice if this does not interfere with installing different GRASS versions in parallel. So we might need to keep grass78 and grass80 anyway? Just to make sure different version in parallel are considered...

@wenzeslaus
Copy link
Member

Have we decided to not use ${MAJOR}${MINOR}? What was wrong with it?

Quoting my initial comment above:

What about dropping the 80 part at least from the documentation? On a reasonable system, it should install as grass anyway. I also noticed I usually write ls and not ls830, gdalinfo, not gdalinfo32... Seems like less work for the releases, too.

In other words, nobody types vim81 or ogr2ogr321. The only exception I can think of is Python where you may have python, python3, python3.8, python2, python2.7, but that's an extreme case. Moreover, it is a packaging issue more than a basic install issue.

Additionally, the current version in the name is misleading and would require a change anyway. Q: What the 80 in grass80 means? A: Perhaps that it was not updated since the 80s.

It has been kept for Windows, and dropped elsewhere.

And why is this inconsistency?

Some Python import issue needs to be resolved to fix the Windows install, but that's not a reason to compromise Linux or macOS. The expectations for executables differ anyway between these platforms.

A version independend startup script is nice if this does not interfere with installing different GRASS versions in parallel. So we might need to keep grass78 and grass80 anyway? Just to make sure different version in parallel are considered...

Quoting earlier comments:

(note that some CI tests will fail as they expect grass79 and not grass80 as the name of startup script)

Can we install just grass to avoid this?

Would be practical, however, I routinely use the stable and the dev versions in parallel. But that could be solved by a link...

Let's not make it hard everywhere and for the users because developers need to have multiple versions.

In other words, the default result you get should be grass because that's expected. If someone is managing different versions, they likely have the skills to deal with that accordingly. There is many ways how parallel versions of one software are managed (symlinks, PATH modifications, Docker, VMs, Python virtual environment, ...), so it is not only about adding version number to the name. This is not to say that GRASS cannot help this in some way, but the default behavior should be that you get a basic grass command in one way or another without any extra steps.

Scripts, CI, etc. should generally just require change of the installed version (Git reference, package version, Docker image, ...) and the rest should work out of the box (assuming no changes in the API), i.e., without the need to go over the files and modify all references to the executable or other things to update the version. This should happen without the need to parametrize the version number and the need to pass it through all the files.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
* html pages: update to GRASS GIS 8.0.dev
* C files: update to GRASS GIS 8.0.dev
* RST files: update to GRASS GIS 8.0.dev
* Python files: update to GRASS GIS 8.0.dev
* core files: update to GRASS GIS 8.0.dev
* misc files: update to GRASS GIS 8.0.dev
* fix broken URL
* db.login manual: fix Windows path
* manual: fix path (Windows)
* grass_database manual: add Windows cmd line startup example
* path name cosmetics
* REQUIREMENTS.html: oldest still-alive Python version is 3.6
* typo fix: depreciated --> deprecated
* startup script: rename grass80 to grass
* added place of rc file on Windows
* gmake8 -> gmake
* grass79 -> grass
* grass80 -> grass
* version: change to generic 8.0.dev
* Windows: have a versionless and versioned startup script
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* html pages: update to GRASS GIS 8.0.dev
* C files: update to GRASS GIS 8.0.dev
* RST files: update to GRASS GIS 8.0.dev
* Python files: update to GRASS GIS 8.0.dev
* core files: update to GRASS GIS 8.0.dev
* misc files: update to GRASS GIS 8.0.dev
* fix broken URL
* db.login manual: fix Windows path
* manual: fix path (Windows)
* grass_database manual: add Windows cmd line startup example
* path name cosmetics
* REQUIREMENTS.html: oldest still-alive Python version is 3.6
* typo fix: depreciated --> deprecated
* startup script: rename grass80 to grass
* added place of rc file on Windows
* gmake8 -> gmake
* grass79 -> grass
* grass80 -> grass
* version: change to generic 8.0.dev
* Windows: have a versionless and versioned startup script
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.

8 participants