-
Notifications
You must be signed in to change notification settings - Fork 147
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
Remove hardcoded versions from release cronjobs #814
base: grass8
Are you sure you want to change the base?
Conversation
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.
No comment on the use of GitHub API, but if you are going with this, the branch-version code needs to be unified across the scripts (as if there would be one script with parameters).
GMAJOR=7 | ||
GMINOR=8 | ||
GPATCH=7 # required by grass-addons-index.sh | ||
BRANCH=`curl https://api.github.com/repos/osgeo/grass/branches | grep releasebranch_7 | grep '"name":' | cut -f4 -d'"' | sort -V | tail -n 1` |
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.
The major version is hidden in this expression, so releasebranch_7
needs to be some branch prefix variable or something.
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.
Right. Another option, to get rid of hardcoded versions completely is to fetch the latest major version -1 to run cron jobs for one version older than current.
That said, maybe also files should be renamed e.g.:
cron_grass7_relbranch_build_binaries.sh to cron_grass_legacy_relbranch_build_binaries.sh
and
cron_grass8_relbranch_build_binaries.sh to cron_grass_latest_relbranch_build_binaries.sh
I probably should also unify how versions are fetched across the different scripts...
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.
In case of renaming of sh-files, changes should be handed down to https://github.com/OSGeo/grass-addons/blob/grass8/utils/cronjobs_osgeo_lxd/README.md and https://github.com/OSGeo/grass-addons/blob/grass8/utils/cronjobs_osgeo_lxd/cron_job_list_grass
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.
Renaming has been done in #814. So, in essence, only https://github.com/OSGeo/grass-addons/blob/grass8/utils/cronjobs_osgeo_lxd/cron_job_list_grass still contains version numbers (log file storage into the right server directory).
utils/cronjobs_osgeo_lxd/README.md
Outdated
* grass7-stable: `cron_grass7_relbranch_src_snapshot.sh` | ||
* grass8-stable: `cron_grass8_relbranch_src_snapshot.sh` | ||
* grass8-devel: `cron_grass8_main_src_snapshot.sh` | ||
* grass-legacy: `cron_grass_legacy_relbranch_src_snapshot.sh` |
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.
...while at it, do you want to use whole words for release branch? The branch names say releasebranch, although they are unfortunately not separated words, they are at least whole words.
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.
So, you mean like this?
- grass-legacy:
cron_grass_legacy_src_snapshot.sh
etc.?
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.
My understanding was (implemented in ac22e46 to fb3fdbd), like renaming from e.g. cron_grass7_relbranch_src_snapshot.sh to cron_grass_legacy_releasebranch_src_snapshot.sh
As for the actual changes in the code: Fetching the version from github (https://raw.githubusercontent.com/osgeo/grass/main/include/VERSION) is tested locally. When reviewing changes, please note that the version is picked up from github at the time the script runs. So, depending on the order in which things are done during release, that may be correct (or not). I don`t know the details of the release procedure, but I am happy to adjust the code if needed.
Also special attention should be put on the patch version. Here only numbers are taken (e.g. dev is removed). From the hardcoded example, that was my understanding how the patch version was defined...
Also I noticed that there is https://grass.osgeo.org/grass-stable/manuals/addons/, but no https://grass.osgeo.org/grass-legacy/manuals/addons/, yet https://grass.osgeo.org/grass7/manuals/addons/. So if we want to support a legacy version, that may be good to add on the webserver...
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 have now activated the grass-legacy/
redirect.
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.
Good! The README.md is adjusted accordingly. However, please note that the version numbers remain in the target log-files in the cron-table (cron_job_list_grass). Ideas how to remove those are welcome...
(... taken care of in #828) |
Hmm... Whoile trying to merge other changes I messed up the PR again. Will cleanup and re-submit a tidy PR once #826 is merged... |
91be659
to
e094f3d
Compare
utils/cronjobs_osgeo_lxd/cron_grass_legacy_releasebranch_build_binaries.sh
Outdated
Show resolved
Hide resolved
utils/cronjobs_osgeo_lxd/cron_grass_legacy_releasebranch_build_binaries.sh
Outdated
Show resolved
Hide resolved
CURRENT_MAJOR=`echo "$CURRENT_VERSION" | sed -n '1{p;q}'` | ||
GMAJOR=`expr $CURRENT_MAJOR - 1` | ||
|
||
BRANCH=`curl https://api.github.com/repos/osgeo/grass/branches | grep releasebranch_$GMAJOR | grep '"name":' | cut -f4 -d'"' | sort -V | tail -n 1` |
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.
This will always result in the same thing as CURRENT_BRANCH since GMAJOR is taken from CURRENT_BRANCH, no?
MAIN_VERSION=`curl https://raw.githubusercontent.com/osgeo/grass/$BRANCH/include/VERSION` | ||
|
||
GMINOR=`echo "$MAIN_VERSION" | sed -n '2{p;q}'` | ||
GPATCH=`echo "$MAIN_VERSION" | sed -n '3{p;q}' | sed 's/[^0-9]*//g'` |
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 don't understand, minor is taken from the file while major is taken from the branch and used for the branch again. Why not getting the right branch and then the version numbers from there?
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.
Is this all just to keep the two scripts in sync? I think the "current" thing is really relevant for the legacy, but here where the code can be simpler. Maybe I just needs to code comments to clarify.
…_binaries.sh Co-authored-by: Vaclav Petras <[email protected]>
…_binaries.sh Co-authored-by: Vaclav Petras <[email protected]>
This PR would need to be updated, following the new scheme (version numbers as of yesterday):
|
After merging of OSGeo#896 and cronjob deployment on grass.osgeo.org various issues came up which are addressed in this PR. Important: - fix incorrect branch in `cron_grass_new_current_build_binaries.sh` - generate programmer's manual only in `cron_grass_new_current_build_binaries.sh` Improvements: - fail early rather than useless continuation (`set -e`) - compile with 2 cores (`make -j2`) - fix copying of `INSTALL` and `REQUIREMENTS.html` to server download directory as being renamed to `INSTALL.md` and `REQUIREMENTS.md` - also copy `CITATION.cff` to server download directory Misc: - no longer generate gettext `.pot` file for transifex since we migrated to https://weblate.osgeo.org/projects/grass-gis/ This PR potentially also addresses OSGeo#814 (can @ninsbl please verify what's yet missing)?
After merging of #896 and cronjob deployment on grass.osgeo.org various issues came up which are addressed in this PR. Important: - fix incorrect branch in `cron_grass_new_current_build_binaries.sh` - generate programmer's manual only in `cron_grass_new_current_build_binaries.sh` Improvements: - fail early rather than useless continuation (`set -e`) - compile with 2 cores (`make -j2`) - fix copying of `INSTALL` and `REQUIREMENTS.html` to server download directory as being renamed to `INSTALL.md` and `REQUIREMENTS.md` - also copy `CITATION.cff` to server download directory Misc: - no longer generate gettext `.pot` file for transifex since we migrated to https://weblate.osgeo.org/projects/grass-gis/ (this PR potentially also addresses #814)
After merging of OSGeo#896 and cronjob deployment on grass.osgeo.org various issues came up which are addressed in this PR. Important: - fix incorrect branch in `cron_grass_new_current_build_binaries.sh` - generate programmer's manual only in `cron_grass_new_current_build_binaries.sh` Improvements: - fail early rather than useless continuation (`set -e`) - compile with 2 cores (`make -j2`) - fix copying of `INSTALL` and `REQUIREMENTS.html` to server download directory as being renamed to `INSTALL.md` and `REQUIREMENTS.md` - also copy `CITATION.cff` to server download directory Misc: - no longer generate gettext `.pot` file for transifex since we migrated to https://weblate.osgeo.org/projects/grass-gis/ (this PR potentially also addresses OSGeo#814)
Fixes #760