rsbivand / rgrass

Interpreted interface between the GRASS geographical information system and R
https://rsbivand.github.io/rgrass/
26 stars 9 forks source link

Future of rgrass - meeting minutes #34

Open veroandreo opened 2 years ago

veroandreo commented 2 years ago

Date: 12/14/2021 Participants: Roger Bivand, Floris Vanderhaeghe, Valentin Lucet, Helmut Kudrnovsky & Veronica Andreo

Package naming: keep the version or drop it? While creating a grass version specific R package makes some sense, i.e., if you have GRASS 7 you use rgrass7, when you use GRASS 8 you use rgrass8; we all liked the idea of a plain rgrass package that might either work for GRASS 7 and 8, or from 8 onward. For this latter option, it was suggested to include a note in the upcoming rgrass7 saying that the update of the package for grass8 will be called rgrass.

Testing This is highly needed. The new rgrass version should have automatic tests to check the code in different operative systems as it is currently in place in GRASS repo through GitHub actions (might use docker images or not). We commented that we could check both against grass main and latest release branches. Valentin commented that he has set such testing environment before and he volunteered to tackle this issue.

Vector data We are all happy with sf being the link for vector data. No changes here, in principle. The use of rsqlite as to link directly to the attribute table is to be revised.

Raster data Several options were discussed:

Side note on rater data: Roger explained that the use of r.in/out.bin was decided because it is "cheaper" to export and import uncompressed data rather than compress-export-uncompress-import. However, other options might be explored in the future.

Parameters and flags Changes in parameter names and flags in GRASS 8 should be accounted for. We need to check the grass changelog for that.

Vignettes The main problem with vignettes is that they would require GRASS and the NC sample dataset installed to be self compiled. This is not possible for a CRAN environment. Valentin said he had some experience with vignettes running uninstalled software. Roger asked if it was possible to get a GRASS binary without gui (In fedora, grass-gui is a separate package, would that help?). We could explore also the possibility of linking to the server where GRASS builds occur, we need to ask. Other option is that vignettes are just “frozen” and state that a certain environment must be set in advance for them to work. This latter seemed like the best option for now.

Where to host the new rgrass version? We discussed several options too: personal github account, r-spatial, grass-addons or new independent organization. We agreed that Roger would create a new repo in his personal account to which we will be invited and then, we could move it to r-spatial subject to their rules. Hosting rgrass in grass-addons repo did not seem like a good option as it is not installed with g.extension in grass, but it is an R package.

Other related topics

Draft roadmap

  1. Create repo and invite others (Roger)
  2. Set up testing (Val)
  3. Drop rgdal support, keep discussing/decide what to do with raster data and implement it
  4. Develop vignettes (Vero will make an attempt)
  5. Drop unused stuff
  6. Include new functionality or enhance existing one to e.g. discover binaries, create locations from epsg codes, etc.

@rsbivand @VLucet @florisvdh @hellik please have a look and let me know if I misunderstood anything. Thanks a lot for your willingness to participate and move forward!

hellik commented 2 years ago

as I've left the meeting earlier, I can't comment on all issues mentioned in the meeting notes.

though, all looks good so far.

@veroandreo I think we should link this discussion to the GRASS user ML; maybe we're able to atttract more interested people.

VLucet commented 2 years ago

Thanks @veroandreo for taking notes! Next meeting I'll take notes so we rotate roles! (sorry I came in late as well).

  1. About hosting & naming: I see @rsbivand renamed the rgrass7 repo to rgrass. I think we should actually keep rgrass7 as it is and open a new repo (named rgrass). This seems cleaner to me (isn't there a risk to breaking urls linking the repo as well?). I think the goal of this new rgrass should be to focus on supporting both GRASS 7 and 8 onward.
  2. I like the idea of a rgrass github org that would host both rgrass7 and the new rgrass. I could move my rgrassdoc there as well. I can imagine other repos being hosted there, like a workshop on applying rgrass (very long term goal of course, I just like making workshops!).
  3. Once we make a decision on naming etc. I can open a PR and set up testing using GitHub actions. I would suggest to start by (1) setting it up with linux/macos/windows with GRASS 7 running simple tests by wrapping the existing examples and (2) adding GRASS 8 to the job matrix and see what breaks and (3) writing better tests!
  4. Unsure about raster and gdal, I don't think I understand the implications of each option well enough.
  5. I like writing vignettes and could help with that of course.
hellik commented 2 years ago
3. Once we make a decision on naming etc. I can open a PR and set up testing using GitHub actions. I would suggest to start by **(1)** setting it up with linux/macos/windows with GRASS 7 running simple tests by wrapping the existing examples and **(2)** adding GRASS 8 to the job matrix and see what breaks and **(3)** writing better tests!

we already have compiling and testing winGRASS in github actions:

though this process has to be updated to reflect OSGeo4W version 2

VLucet commented 2 years ago

we already have compiling and testing winGRASS in github actions:

Cool, that means I don't have to figure out how to install it on windows (action wise)! Would be cool to have it as a standalone action, I'd just have to write uses:. Could be another goal down the line.

hellik commented 2 years ago

we already have compiling

compiling winGRASS to testing rgrass may not be needed as precompiled winGRASS can be downloaded via github action. e.g.

example download

Start-Process ('.\'+$exe) -ArgumentList '-A -g -k -q -s http://download.osgeo.org/x86_64 -P cairo,fftw,freetype-devel,gdal-ecw,gdal-mrsid,liblas-devel,libxdr,msys,pdcurses,python3-numpy,python3-pywin32,python3-wx,regex-devel,wxpython,zstd-devel' -Wait

rsbivand commented 2 years ago

@VLucet Yes, I thought about the architecture of one or two repos. After (maybe too little) consideration, I changed the name of the repo rather than starting a new repo. My idea (maybe wrong) is that 1) I switched the default branch from master to main, and changed the URL in DESCRIPTION to point to rgrass rather than rgrass7 in main. So a release soon would be good (the current state checks OK with 7.8.6 and 8.0dev).

Next, we make an rgrass7 branch, which will be the target maintenance source for the "old" package. Until the rgrass branch is ready for release, mainand rgrass7 are the same (changes on rgrass7 are merged into main). When rgrass is ready for submission to CRAN, rgrass is merged with main, and rgrass7 is updated to give a startup message advising users to switch to rgrass.

I thought that there is so much shared code that we risk missing maintenance updates if the codebase is split over two repos. Does this make sense?

So testing would use main in principle, but could use main and rgrass before the switch, main and rgrass7 afterwards if need be.

Then when we are ready, we can create an organisation and move the rgrass and rgrassdoc repos there.

VLucet commented 2 years ago

@rsbivand Ah yes I see! That indeed makes a lot more sense in terms of maintenance, I completely agree.

florisvdh commented 2 years ago

Thank you @veroandreo for the minutes, they look complete to me.

@rsbivand yes, I also think it's sensible to use the three branches in order to maintain the two package flavours. To avoid confusion, maybe the definition of the branch contents could be put in a file .github/CONTRIBUTING.md? (It's a file also picked up by pkgdown, linked as 'Contributing guide' in the sidebar).

I think that main - although equal to rgrass7 for the time being - will also contribute to rgrass branch, if only for new organizational additions (like yesterday's commits, perhaps a few contributing guidelines), that are common to both rgrass and rgrass7 package source. There might even be a need for a separate branch (instead of main) with the common base for rgrass7 and rgrass branches, while the specific stuff happens in the latter branches (especially rgrass obviously):

VLucet commented 2 years ago

I agree with @florisvdh for the CONTRIBUTING file. Maybe I'm missing something, but as long as the rgrass branch can be rebased onmain every now and then (commits could be cherrypicked) we should not have a need for another separate branch, A main, rgrass7 and rgrass set of branches makes sense to me.

VLucet commented 2 years ago

I just wanted to start the conversation on this again, and ask whether we have decided what set of branches we'd like to work with, and if yes, whether I can get started with setting up github actions on the main and rgrass branches.

florisvdh commented 2 years ago

As far as I'm concerned, please feel free to choose the setup that you expect will work best. My suggestions were just meant to aid the thinking process!

rsbivand commented 2 years ago

Could someone add the CONTRIBUTING file to main, then I'll create the two branches, and move towards releasing rgrass7 from this repo . The release may be the last, as rgrass will not stop supporting GRASS 7, but will not develop, I think.

veroandreo commented 2 years ago

Could someone add the CONTRIBUTING file to main, then I'll create the two branches, and move towards releasing rgrass7 from this repo . The release may be the last, as rgrass will not stop supporting GRASS 7, but will not develop, I think.

Indeed. We are really close to finally release GRASS 8. At the moment there's already RC2; meaning that 8 will soon become the stable version and 7 will only receive bug fixing but no new features. So, trying to get a working version of rgrass for GRASS 8 would be great :)

rsbivand commented 2 years ago

The 0.2-7 rgrass7 on the rgrass7 and main branches and the 0.3-1 rgrass on the rgrass branch both pass R CMD check --as-cran for both of 7.8.7RC1 and 8.0.0RC2 (locally built, Fedora 35). If soneone else would like to confirm, possibly on some other platform, then I suggest submitting rgrass7_0.2-7 soon. When 8.0.0 is released, this rgrass7 version will work. When 8* stabilizes, we can move to submit rgrass and once published, can add a suggestion to migrate to the rgrass7 startup mesage.

VLucet commented 2 years ago

Could someone add the CONTRIBUTING file to main

I've opened a PR for this see #38. I can corfirm the package passes checks fine with 7.8 and, as far as I can tell, with 8.0.

I've also branched out of main to start setting up actions.

florisvdh commented 2 years ago

The rgrass7 branch (already merged in main) can now be updated to main (rebase or merge -ff) and pushed in order to keep both on par. The rgrass branch can receive the updates from main (or be rebased on it), and pushed. I'm proposing that also in order to double check there remains no misunderstanding about the workflows!

florisvdh commented 2 years ago

The 0.2-7 rgrass7 on the rgrass7 and main branches and the 0.3-1 rgrass on the rgrass branch both pass R CMD check --as-cran for both of 7.8.7RC1 and 8.0.0RC2 (locally built, Fedora 35).

@rsbivand I get following logs from R CMD check --as-cran under R 4.1.2, Linux Mint 20 (Ubuntu Focal):

On the command line I did R CMD build . followed by R CMD check <tarball> --as-cran. The result seems to be confirmed by rcmdcheck::rcmdcheck(args = "as-cran") in R, perhaps with slight differences in the presentation.

rsbivand commented 2 years ago

Most likely the setup. Please avoid all packages claiming to help, they only get in the way, this is probably where the crashes are coming from. The extra tarball shouldn't be there, something is making assumptions about which directory it thinks you are in.

From the directory above the git repo (in rgrass7 or rgrass branch), run R CMD build rgrass and check that the tarball name agrees. Then start GRASS (8.0.0RC2 or 7.8.7RC1) in that directory in nc_basic_spm_grass7, and inside GRASS run `R CMD check .tar.gz.

Latex is needed for manuals, if unavailable use --no-manual.

rsbivand commented 2 years ago

Is the double free or corrupt related to https://lists.osgeo.org/pipermail/grass-user/2022-January/082834.html ?

florisvdh commented 2 years ago

Thank you for the hints Roger. Also, it was interesting to learn compiling and installing GRASS 7.8.7RC1 and 8.0.0RC2 alongside the existing binary 7.8.6 installation from ubuntugis-unstable.

The check with rgrass7 + GRASS 7.8.7RC1 now runs fine here (previously I only had GRASS 7.8.6 from ubuntugis-unstable). Logfile (for now, used --no-manual) with 1 NOTE. The double free or corrupt is gone, and yes as hinted in the grass-user mail problems have arisen because rgdal wasn't yet recompiled with the recently updated GDAL/PROJ/GEOS from ubuntugis-unstable (with GRASS 7.8.7RC1, this also gave a segfault when crs was called in an example; after recompiling rgdal this was gone).

Same (no errors) for rgrass + GRASS 8.0.0RC2 - logfile with 3 NOTES.

Note to self (prompts and command for both cases):

GRASS 7.8.7RC1 (nc_basic_spm_grass7):/media/floris/DATA/git_repositories > R CMD check --as-cran --no-manual rgrass7_0.2-7.tar.gz

GRASS nc_basic_spm_grass7/PERMANENT:git_repositories > R CMD check --as-cran --no-manual rgrass_0.3-1.tar.gz
rsbivand commented 2 years ago

rgrass7_0.2-7 submitted to CRAN.

rsbivand commented 2 years ago

Source and some binaries (not yet all) rgrass7_0.2-7 on CRAN.

florisvdh commented 1 year ago

Hi @rsbivand we think the meeting minutes, like those of yesterday, will better fit in GitHub discussions than in GH issues. Could you activate the Discussions tab in the Settings of the repo?

Also I guess current issue can be closed since things have evolved quite a lot since Dec 2021.

rsbivand commented 1 year ago

I opened discussions, I think. Its tinsel really irritated me. Can others open discussions, or only the repo owner?

florisvdh commented 1 year ago

Yes, it seems that opening a new discussion will work, at least I can, but probably anyone. Thanks.