rsbivand / rgrass

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

Maintenance ideas #61

Open florisvdh opened 1 year ago

florisvdh commented 1 year ago

Trying to catch up; congratulations Roger with the rgrass release on CRAN :heart: !

I think the state of .github/CONTRIBUTING.md has become out of date: main is now the branch representing the rgrass package, making the rgrass branch obsolete. Further, all branches except val-adding-github-actions (not figured below) are inside the history of the main branch (branch tips are boldface below):

$ git log main --format='%C(auto)%h (%ai)%d %s' -n 35
e9bf6a0 (2022-08-08 15:55:34 +0200) (HEAD -> main, origin/main, origin/HEAD) update docs
aab0aa9 (2022-08-08 09:13:07 +0200) tidy
fe1f9e4 (2022-08-01 17:31:00 +0200) attempt fix to M1 vignette issue
a50385b (2022-07-21 16:31:25 +0200) Merge pull request #60 from rsbivand/rgrass
b87ec74 (2022-07-21 16:29:48 +0200) (origin/rgrass) Merge pull request #59 from rsbivand/main
083c88d (2022-07-21 15:09:17 +0200) update docs
190366e (2022-07-21 12:22:51 +0200) update docs
406d3da (2022-07-21 12:18:05 +0200) update docs
23d99e3 (2022-07-21 11:27:52 +0200) update docs
3971142 (2022-07-21 11:26:20 +0200) update docs
323e193 (2022-07-21 11:21:52 +0200) tidying up to meet submission requirements
60bf284 (2022-07-20 14:46:05 +0200) Merge pull request #58 from rsbivand/rgrass
193d55f (2022-07-20 14:37:42 +0200) update docs and vignettes
9bc1e0c (2022-07-20 08:51:16 +0200) complete coerce vignette
b75e863 (2022-07-12 17:39:31 +0200) starting vignettes
b3d741a (2022-06-21 15:18:29 +0200) prepare for rgrass submission
eb2fec6 (2022-05-02 10:23:12 +0200) revert version number
a118be5 (2022-05-02 10:20:01 +0200) (origin/rgrass7, origin/fix-emails) fix error in email format
249c1ab (2022-04-08 14:13:18 +0200) Merge pull request #56 from rsbivand/init_no_SG_53
4bd201f (2022-04-08 14:12:55 +0200) (origin/init_no_SG_53) Merge branch 'rgrass' into init_no_SG_53
a0ea76d (2022-04-08 14:10:49 +0200) Merge pull request #55 from rsbivand/rgrass7
f867ff3 (2022-04-08 14:10:02 +0200) Merge pull request #54 from rsbivand/init_no_SG_53
c27b803 (2022-04-08 14:09:44 +0200) addressing #53
e6cba19 (2022-04-08 13:58:40 +0200) addressing #53 - sp SG
087446c (2022-04-08 10:53:15 +0200) addressing #53
aa531c0 (2022-04-08 10:34:57 +0200) addressing #53
927b34e (2022-03-05 11:13:55 +0100) Merge pull request #50 from rsbivand/rgrass7
37ab6e2 (2022-03-05 11:07:02 +0100) tidy
f1fe032 (2022-03-05 11:03:16 +0100) Merge branch 'main' into rgrass7
7ee573d (2022-03-05 11:01:09 +0100) Merge branch 'rastNG' into rgrass
1253286 (2022-03-05 10:58:40 +0100) Merge branch 'rgrass' of github.com:rsbivand/rgrass into rgrass7
89c57b1 (2022-03-05 10:57:24 +0100) (origin/rastNG) Merge branch 'rastNG' of github.com:rsbivand/rgrass into rastNG
10faeb8 (2022-03-05 10:56:37 +0100) fix class() inherits() NOTE
ca92c38 (2022-03-04 17:53:25 +0100) update docs
f6d4543 (2022-03-04 17:50:52 +0100) update docs

We still need the rgrass7 branch, for adding a commit (specific to that branch) to address the last part of CONTRIBUTING.md: rgrass7 will be updated to give a startup message advising users to switch to rgrass. This remains to be done, and it will result in the rgrass7 branch having a unique commit, hence departing from main (which is logical).

Also, this means that the other branches (except val-adding-github-actions) can be deleted in order to simplify things (without losing anything). The branches are just dynamic pointers to a commit; commits only get lost if they don't belong to any branch's history. What is needed if you want to pinpoint certain commits, are git tags, in order to pinpoint the CRAN releases. Some example git code:

git tag rgrass7_0.2-10 a118be5  # supposing this is the commit that was submitted
git tag rgrass_0.3-3 e9bf6a0 # supposing this is the commit that was submitted
git push --tags 

The tags, if you agree that this would be useful:

After pushing, they will also show up at https://github.com/rsbivand/rgrass/tags. (And they form the basis for 'github releases' if you wish to use that, optionally triggering an automated deposit at Zenodo, with which I could help).

Suggestions for now (some may have been your plan anyway):

Please let me know what you think and whether I can help in this.

rsbivand commented 1 year ago

I'm SCSS/CVS/SVN, don't use branches in most cases, no tags, no releases. My MO is to commit locally until the local version passes CMD check --as-cran locally on R released and devel, with my GRASS (sometimes old GRASS and/or devel GRASS as well as stable), then begin a submission process and push to the repo. Even when published on CRAN, a second release may be needed (0.3-1 didn't pass CRAN submission procedures, 0.3-2 did but with a logic error when a suggested package was absent, hence 0.3-3). So using github processes is a distraction since github is not the release repo. The same with deleting dead branches, for me they were kept to document steps. I agree that CONTRIBUTING could be revised (I never read any such documents) if anyone reads it. Also I agree that after FOSS4G rgrass7 should be released, but am unsure whether the .Deprecated() mechanism should be used or not, I think probably it should https://www.stat.ethz.ch/R-manual/R-devel/library/base/html/Deprecated.html .

florisvdh commented 1 year ago

Hi Roger, of course the maintainer decides what to use or not! I agree that GitHub releases could be confounding wrt CRAN releases and would just duplicate things. Git tags are just a git facility (not GitHub's) and meant for documenting history. FWIW, branching history remains stored also after deleting a merged branch (deleting essentially drops the reference to a commit), e.g.:

$ git log --oneline --graph --decorate -n 8
* c86431f (HEAD -> main, origin/main, origin/HEAD) Update README.md
* dd21247 Update CONTRIBUTING.md
*   374301a Merge pull request #62 from rsbivand/somefixes
|\  
| * 0a600f0 Update pkgdown website home & news accordingly
| * 29cd8f0 NEWS: minor updates
| * edad5cf README: update url syntaxis
|/  
* e9bf6a0 update docs
* aab0aa9 tidy

I just noticed that you did delete the old (merged) git branches – of course it's fine keeping them if that suits you better. It's partly a matter of preference; I normally regard existing branches as 'still developing'. Some more info e.g. at https://stackoverflow.com/q/1457103.

Quite some R packages do use GitHub / pkgdown to enhance visibility / accessibility for users or developers, but indeed it is not necessary to do so. The contributing guide is linked in the right column of the package homepage.

florisvdh commented 1 year ago

Regarding .Deprecated(), I'm simply not familiar enough with it, but it seems targeted at functions, rather than package level.

rsbivand commented 1 year ago

Yes, it is for functions. I'd thought of applying it to the file interchange functions.

rsbivand commented 1 year ago

Re. github releases - they may make sense if the R package does not critically depend on (versioned) upstream packages, here XML and terra, possibly others. Setting up CI I think stalled, and designing forward-looking CI is really hard (accommodating possible future changes in upstream packages). This means adding code to condition on versions of packages, and is really only feasible locally. This doesn't affect this package too much, but certainly does for those I maintain on r-spatial.

What should we do about CI?

florisvdh commented 1 year ago

The GitHub releases are pure GitHub facility; I wouldn't give it much attention relative to CRAN. I had only mentioned it superficially since they depend on git tags, while the latter have merits by themselves, i.e. tagging specific commits with a meaningful label (in the local git repo, and distributed through the remote), for the sake of documentation or finding back things later on etc. Git tags are agnostic of the chosen remote server infrastructure (be it GitHub or another git server e.g. GitLab, which is open source).

florisvdh commented 1 year ago

CI: to me it has clear advantages in collaborative settings with a minimum activity of contributors, in the sense that it makes it easier both for reviewers and contributors to have auto-running R CMD check easily, typically in pull requests from contributors.

However the need for CI is not high at the moment since we are not in a situation of frequent active contributions, so there's no urgency IMHO.

@RobinLovelace has experience with Docker containers having the external geospatial software; in pull requests I think the GeocompR book is built both against CRAN releases (workflow file of an example run) and against unreleased versions of sf, stars and terra (workflow file of an example run).

florisvdh commented 1 year ago

@VLucet can you give an update on the working state of the CI implementation? It seems that you already added a GitHub action workflow that calls a GRASS action that you made, in your branch val-adding-github-actions. Also a typical workflow to perform a package check.

Robinlovelace commented 1 year ago

@Robinlovelace has experience with Docker containers having the external geospatial software; in pull requests I think the GeocompR book is built both against CRAN releases (workflow file of an example run) and against unreleased versions of sf, stars and terra (workflow file of an example run).

Confirmed although I'm no Docker expert. In case it helps, here's our workflow: https://github.com/Robinlovelace/geocompr/blob/7697331ada5442c3da9ffe97d99b6bf7f38d2f4c/.github/workflows/dev-sf.yaml#L11-L25

You could run it on the geocompr/geocompr:qgis-ext container as shown here: https://github.com/Robinlovelace/geocompr/blob/qgis/.github/workflows/qgis-ext.yaml

That has GRASS installed so may be good for testing?

VLucet commented 1 year ago

Hi all apologies for the delay I've been busy hitting dealdines. I will pick this up next week, but the gist is that I was trying to first write an action that would build the latest from GRASS and then install the package and test that, but was struggling to make it work across platforms.

@Robinlovelace I wasn't aware of that container and first attempted to build GRASS myself from the latest code! I will take a look as soon as Im able to!