-
Notifications
You must be signed in to change notification settings - Fork 185
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
Adjustments for non-GitHub hosters #399
Adjustments for non-GitHub hosters #399
Conversation
7a9da25
to
8cfe7c1
Compare
This is now in a state where I believe that it is ready to be reviewed, for it allows building off-GitHub pages branches such as the one shown at https://chrysn.codeberg.page/packed-by-reference/. Using it still requires some changes in the repository's local .github/workflows/ghpages, and there are missing features (no saved issues) and rough edges (the string "GitHub" is not hosterized yet the way the URIs are), but it produces useful output. (It will be more useful when author-tools.ietf.org allow-lists more page hosters). |
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.
Pretty easy to take this one, I think. Just nits.
I would consider variable name changes here. Mostly, these are unlikely to be used by anyone, so a change in name is probably fine.
else | ||
GITHUB_REPO_FULL := $(CI_REPO_FULL) | ||
CI_HOST ?= github.com |
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.
You don't use this, but it might pay to do something about the variable names at some point (a follow-up perhaps).
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'm not using it, but frankly I don't understand in which situation CI_* are set -- but I'd like to give whomever uses it the opportunity to overwrite what eventually goes into the hoster argument of build-index.sh.
ghpages.mk
Outdated
@@ -36,19 +36,23 @@ PUSH_GHPAGES ?= true | |||
endif | |||
PUSH_GHPAGES ?= false | |||
|
|||
# Allow overriding the pages branch. It is gh-pages on GitHub, but pages on | |||
# Forgejo (currently not auto-detected). | |||
PAGESBRANCH ?= gh-pages |
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.
It seems like this could be detected easily enough.
Also, I might prefer PAGES_BRANCH instead, if only to add some amount of consistency.)
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.
Renamed (in an insta-squashed fixup).
As for detection: Yes it can, now that GITHUB_HOST is available (I missed that this has now become actionable), added on top. One fewer hack in the patched ghpages.yaml :-)
This introduces a currently unused GITHUB_REPO_WITHHOST variable that contains the normalized (i.e. slashes everywhere, no .git) github.com/user/repo part, which may later be useful to pick URI templates for non-GitHub hosters.
This will be github.com for GitHub hosted repositories, but will allow adjustments to URI templates as necessary for other hosters. This also fixes the documentation of build-index.sh to account for the branch argument.
As there is no generic mapping from the github.com style addresses to the github.io style addresses, a switch based on the hoster decides what to do, with some not-too-far-off (but not actually used) default for unknown hosters.
I've taken the liberty to add one more unprompted change: generation of .note.xml is trivial to adjust to non-GitHub hosters. (I've previously missed that because I'm using |
Thanks for all of this. These are easy changes to take as they are small and targeted. |
This collects contributions to #398, and will stay a draft PR until it's somewhat usable.