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

[ci] use GitHub Actions to re-generate R configure #4140

Merged
merged 2 commits into from
Mar 31, 2021
Merged

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented Mar 30, 2021

Closed #3283.

Refer to #4117 (review) for the initial discussion.

Refer to https://github.com/StrikerRUS/LightGBM/pull/1 for the usage example.

Push from a workflow won't trigger our regular workflows:

For example, if a workflow run pushes code using the repository's GITHUB_TOKEN, a new workflow will not run even when the repository contains a workflow configured to run when push events occur.
https://docs.github.com/en/actions/reference/events-that-trigger-workflows#triggering-new-workflows-using-a-personal-access-token

I don't think that this a problem for us, because anyway we need a follow-up commit with reverting back to ~~VERSION~~ in configure.ac, right?

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

because anyway we need a follow-up commit with reverting back to VERSION in configure.ac, right?

No, no followup commit is required because there is never a commit that removes the ~~VERSION~~ in configure.ac. I'm confused, how is configure.ac getting updated and showing up in the diff for https://github.com/StrikerRUS/LightGBM/pull/2? Looking at the workflow template, I don't understand how configure.ac is getting changed OR how that change is getting committed.

Notice it was unchanged in the diff for the most recent release: https://github.com/microsoft/LightGBM/pull/3872/files, and recreate-configure.sh does that templating with a temporary configure.ac:

cp configure.ac ${TMP_CONFIGURE_AC}
sed -i.bak -e "s/~~VERSION~~/${LGB_VERSION}/" ${TMP_CONFIGURE_AC}
.

.github/workflows/r_configure.yml Outdated Show resolved Hide resolved
@StrikerRUS
Copy link
Collaborator Author

I'm confused, how is configure.ac getting updated and showing up in the diff for StrikerRUS#2?

Just like it is suggested in https://github.com/microsoft/LightGBM/tree/master/R-package#changing-the-cran-package (Linux or Mac section):

  1. edit configure.ac
  2. comment /gha run r-configure
  3. revert step #1

How this process should be done right?

@jameslamb
Copy link
Collaborator

"Edit configure.ac" there is not about changing the version. It's about making functional changes, for example like the one suggested in #4131.

If the only thing that is changing is the version of LightGBM, you do not need to touch configure.ac at all.

@StrikerRUS
Copy link
Collaborator Author

If the only thing that is changing is the version of LightGBM, you do not need to touch configure.ac at all.

OK, got it. What should be touched then?

@jameslamb
Copy link
Collaborator

If the only thing that is changing is the version of LightGBM, you do not need to touch configure.ac at all.

OK, got it. What should be touched then?

you don't need to manually edit any files. Just running recreate-configure.sh will create a new configure script.

@StrikerRUS
Copy link
Collaborator Author

you don't need to manually edit any files.

Ah, seems I'm getting what's going on! Just manually edit VERSION.txt, right?

@jameslamb
Copy link
Collaborator

Yep! Just manually edit VERSION.txt if that's changing. Which I think is ok.

Sorry if the instructions weren't clear. When I wrote https://github.com/microsoft/LightGBM/tree/master/R-package#changing-the-cran-package , the main audience I had in mind was contributors making changes like #4131, not our usual maintenance like doing a new release. If there are changes to the language there that you think would make it more clear, I'd be happy to review those as part of this PR.

@StrikerRUS StrikerRUS changed the title [ci] use GitHub Actions to re-generate R configure [WIP][ci] use GitHub Actions to re-generate R configure Mar 30, 2021
@StrikerRUS StrikerRUS force-pushed the r_configure branch 2 times, most recently from e86cde1 to bf84c94 Compare March 30, 2021 22:07
@StrikerRUS StrikerRUS changed the title [WIP][ci] use GitHub Actions to re-generate R configure [ci] use GitHub Actions to re-generate R configure Mar 30, 2021
@StrikerRUS
Copy link
Collaborator Author

OK, @jameslamb please take one more look. I hope I understood it right this time.
Here is new demo: https://github.com/StrikerRUS/LightGBM/pull/1.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

This looks great, thanks! The diff in https://github.com/StrikerRUS/LightGBM/pull/1/files#diff-dadfc37f082f218f0e2f0adfc1affc7c55044bcdf8dff110aab9cdfda842d6f3 looks exactly like what I'd expect. Just one minor grammar comment, but I think the workflow is perfect.

I think it is ok that the commit from this bot won't trigger a new CI build. Usually on release PRs (the main place where we update configure) there are additional commits after updating configure that don't have any impact on configure.

For cases like #4131 , this will have to be something that we "just know" to re-run if the contributor doesn't manually regenerate configure. I don't think that's problematic at all, as that file very rarely receives updates that are not part of the release process: https://github.com/microsoft/LightGBM/commits/master/R-package/configure.

R-package/README.md Outdated Show resolved Hide resolved
run: |
git config --global user.name "GitHub Actions Bot"
git config --global user.email "githubactionsbot@users.noreply.github.com"
git add "./R-package/configure"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can tell your main OS is Windows because of your defensive use of quotes 😛

@StrikerRUS
Copy link
Collaborator Author

Thanks!

looks exactly like what I'd expect.

Great!

I think it is ok that the commit from this bot won't trigger a new CI build

It does with the latest code! 🙂 Thanks to token and persist-credentials arguments of actions/checkout.

image

For cases like #4131 , ...

Yeah, unfortunately, GitHub Actions cannot push into forked repos. So we can't do anything with it.

Co-authored-by: James Lamb <jaylamb20@gmail.com>
@StrikerRUS StrikerRUS merged commit 9388b2e into master Mar 31, 2021
@StrikerRUS StrikerRUS deleted the r_configure branch March 31, 2021 11:37
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R-package] automate release process for R
2 participants