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

Maintenance ideas #61

Open
florisvdh opened this issue Aug 12, 2022 · 10 comments
Open

Maintenance ideas #61

florisvdh opened this issue Aug 12, 2022 · 10 comments

Comments

@florisvdh
Copy link
Collaborator

florisvdh commented Aug 12, 2022

Trying to catch up; congratulations Roger with the rgrass release on CRAN ❤️ !

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:

  • will allow being enlisted in git, e.g.:
$ git tag -l --format='[%(authordate:iso8601-strict) %(authorname)] %(objectname:short) %(refname:strip=2)'
[2022-05-02T10:20:01+02:00 Roger Bivand] a118be5 rgrass7_0.2-10
[2022-08-08T15:55:34+02:00 Roger Bivand] e9bf6a0 rgrass_0.3-3

or (with syntax highlighting as well as matching branch names):

$ git log --tags --no-walk --format='%C(auto)%h (%ai) %an %D'
e9bf6a0 (2022-08-08 15:55:34 +0200) Roger Bivand tag: rgrass_0.3-3, origin/main, origin/HEAD, main
a118be5 (2022-05-02 10:20:01 +0200) Roger Bivand HEAD, tag: rgrass7_0.2-10, origin/rgrass7, origin/fix-emails
  • will allow a checkout based on the tag name, e.g. git checkout rgrass7_0.2-10

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):

  • branches that are inside the history of another one (were merged into it), can be safely deleted. I advise to delete all branches except main, rgrass7 and val-adding-github-actions. It can be done in either https://github.com/rsbivand/rgrass/branches or from the shell (e.g. git push --delete origin/rgrass).
  • tag CRAN releases and push tags (see above)
  • making a new rgrass7 release that shows a startup message to make the user aware of the rgrass package. The rgrass7 branch could possibly still evolve further after that, if fixes that occur in main that also apply to the rgrass7 package, are backported to the rgrass7 branch.
  • simplifying CONTRIBUTING.md, referring to main as the sole branch to contribute to in the future (of course a contributor should making a separate feature branch, adding proposed commits there and then make a PR towards main).
  • maybe time is ready to close issues new rgrass for the upcoming grass version 8? #28 and Future of rgrass - meeting minutes #34

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

@rsbivand
Copy link
Owner

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
Copy link
Collaborator Author

florisvdh commented Aug 16, 2022

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
Copy link
Collaborator Author

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

@rsbivand
Copy link
Owner

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

@rsbivand
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

  • Just implementing a package check without actual GRASS installation involved is not too difficult (at least when implementing existing actions prepared by the r-lib team).
  • Also setting R packages at a fixed or minimum version for that purpose (e.g. using renv) should be doable.
  • However more usefulness would come if also GRASS is incorporated and used in the check (which would probably need Docker containers with GRASS installed). I could dive into that (I have minimal experience with GitHub actions, practically none with Docker) but I'm afraid I don't have the time during this year, at least for the last option with Docker & GRASS.

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
Copy link
Collaborator Author

florisvdh commented Aug 16, 2022

@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
Copy link
Contributor

@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
Copy link
Collaborator

VLucet commented Aug 31, 2022

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!

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

No branches or pull requests

4 participants