Skip to content

Conversation

@jgfoster
Copy link
Contributor

git submodule add needs the branch before the repository or else it is ignored. The previous code checked out the master branch, not the stable branch.

`git submodule add` needs the branch before the repository or else it is ignored. The previous code checked out the `master` branch, not the `stable` branch.
@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Nov 24, 2020

Do you mind bumping jwlawson/actions-setup-cmake@v1.3 to 1.4 in .github/workflows/configure? (cfr. this line in #2656, which is not part of stable yet) Alternatively, target master, I suppose.

@jgfoster
Copy link
Contributor Author

@YannickJadoul, I'm new to pybind, I found a problem with the docs, did an on-line edit, and submitted a PR. I don't really know anything about the GitHub workflows for this project and it seems like you are requesting a change that has nothing to do with fixing the documentation. Is there a reason you think I should make this change and do so as part of this PR? I think it would make more sense for someone who understands your request to open an issue or submit a new PR. Is that reasonable? Is there something I'm missing? If your suggestion is to help get tests to pass, that is fine, but my change didn't break them and my knowledge is sufficiently thin that I don't think I should be making other changes.

@YannickJadoul
Copy link
Collaborator

Is there a reason you think I should make this change and do so as part of this PR?

Well, yeah, the idea was to make the CI pass, indeed. There's absolutely no way your change caused this, but by targeting stable, you have apparently shown the CI on stable is outdated. Preferably, we don't merge a PR before it turns green (if only because our stable will suddenly be red).

I can make that change quickly as well, but let's ask @henryiii if this PR should be targeted to stable or master.

@henryiii
Copy link
Collaborator

henryiii commented Nov 24, 2020

It's really up to @wjakob, but I think we should always target master, and then update stable for new releases. For docs, it is potentially a bit more complicated, since stable is what shows up by default in readthedocs, so a docs-only PR could in theory target stable. But if that's the case, it should still target master, and should be backported to stable with some sort of note or label.

And, in this case, we’d have to backport our CI changes to stable too, since stable has the old actions that were updated for the security patch.

@henryiii henryiii changed the base branch from stable to master November 24, 2020 16:33
@henryiii henryiii closed this Nov 24, 2020
@henryiii henryiii reopened this Nov 24, 2020
@henryiii henryiii added the docs Docs or GitHub info label Nov 24, 2020
Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows jobs randomly have an error due to a known issue - that's fine. LGTM.

@henryiii
Copy link
Collaborator

This won't show up in the "stable" branch on RtD until we have a release that includes it, but it will change on the "latest" docs immediately. Thanks!

@henryiii henryiii merged commit d57c1fa into pybind:master Nov 24, 2020
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 24, 2020
@jgfoster jgfoster deleted the patch-1 branch November 24, 2020 17:14
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Docs or GitHub info

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants