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

[DO NOT MERGE] Release 3.3.2 #4930

Closed
wants to merge 5 commits into from
Closed

[DO NOT MERGE] Release 3.3.2 #4930

wants to merge 5 commits into from

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented Jan 6, 2022

This is unusual release because it doesn't include all changes made since v3.3.1 release in master, but only contains urgent maintenance change to make LightGBM compatible with R 4.2. Refer to #4923 for more details.

Tags and artifacts are made directly from release_332 branch instead of master as usual, and this PR shouldn't be ever merged.

Release checklist:

  • Update VERSION.txt number.
  • Update version in Appveyor config file.
  • Update version in configure file of R-package: /gha run r-configure.
  • Change development.mode from unreleased to release in pkgdown config file.
  • [ ] Run R valgrind checks after all PRs are merged: /gha run r-valgrind. (cannot be run due to conflicts with master)
  • [ ] Run R Solaris checks after all PRs are merged: /gha run r-solaris. (cannot be run due to conflicts with master)
  • Push commit with v* tag to trigger GitHubRelease action at Azure Pipelines.
  • Convert automatic GitHub release from Draft to normal one.
  • Update stable tag at GitHub.
  • Trigger RTD build for new tag.
  • Upload release to PyPI.
  • Upload release to CRAN.
  • Update cran-comments.md when new release is accepted on CRAN.
  • Upload release to NuGet.
  • Update version and commit hash in Homebrew formula.
old changelog might be needed in the future

Changes

💡 New Features

🔨 Breaking

🚀 Efficiency Improvement

🐛 Bug Fixes

📖 Documentation

🧰 Maintenance

jameslamb and others added 3 commits January 6, 2022 17:26
* [docs] [R-package] update cran-comments for v3.3.1 release

* Update R-package/cran-comments.md

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>

* update cran-comments.md now that v3.3.1 is accepted

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@StrikerRUS
Copy link
Collaborator Author

/gha run r-configure

@StrikerRUS StrikerRUS changed the title Release 3.3.2 [DO NOT MERGE] Release 3.3.2 Jan 6, 2022
* pin Dask version at CI

* Update .vsts-ci.yml

* Update .vsts-ci.yml

* workaround for Python 3.6

* Update test.sh
@StrikerRUS StrikerRUS marked this pull request as ready for review January 6, 2022 17:56
@StrikerRUS
Copy link
Collaborator Author

StrikerRUS commented Jan 6, 2022

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.

Thanks for doing this!

Ok with me that we don't have the valgrind and solaris checks on this PR, since they ran successfully on #4923. I pull this branch today just to double-check, and can confirm (from running git log -n 10) that this only contains the new patch applied on top of v3.3.1.

One thing I'm unsure about is the release tag. Will we have to permanently keep this release_332 branch in the repo from this point forward? Since the tagged commit (dce7e58) will never be on master.

If so I think that's fine, but we should add a branch protection on release_332 to prevent deleting it or pushing changes to it without a pull request (GitHub docs). I don't have sufficient permissions to do that on this repo, so it will have to be you (if you have the permissions) or @shiyu1994.

@jameslamb
Copy link
Collaborator

Once @shiyu1994 approves as well, I can handle uploading this release to CRAN.

@StrikerRUS
Copy link
Collaborator Author

@jameslamb

Will we have to permanently keep this release_332 branch in the repo from this point forward?

I'll check this.

Copy link
Collaborator

@shiyu1994 shiyu1994 left a comment

Choose a reason for hiding this comment

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

LGTM. @StrikerRUS @jameslamb Thanks you!

@StrikerRUS
Copy link
Collaborator Author

OK, thanks for the feedback, guys!

New GitHub release is out now and available for public access.

@jameslamb
Copy link
Collaborator

Excellent! I'll submit to CRAN shortly and let you both know here when I do.

I think we can close this pull request.

@StrikerRUS
Copy link
Collaborator Author

Yeah, thanks!

Everything is done according to the release checklist, except CRAN upload.

Homebrew formula is under review right now: Homebrew/homebrew-core#92706.

I'll submit to CRAN shortly and let you both know here when I do.

I believe we should remind them to remove auto-patching in comments during upload.

@StrikerRUS StrikerRUS closed this Jan 7, 2022
@jameslamb
Copy link
Collaborator

I believe we should remind them to remove auto-patching in comments during upload.

Good point! I'll do that.

@StrikerRUS
Copy link
Collaborator Author

Will we have to permanently keep this release_332 branch in the repo from this point forward?

I'll check this.

I've checked this and it seems we can remove underlying branch. I created test branch with test commit. Then I pushed test tag referring test commit. After that I removed entire test branch. And here's what I've got:

image

image

@jameslamb
Copy link
Collaborator

I've checked this and it seems we can remove underlying branch

I'm nervous about this. I've seen that when you delete a branch in the UI, there is a period of time where GitHub keeps it around and you can click "restore branch" to bring it back. Then, after some time (I do not know how long, but on the order of hours or days, not weeks or months), that's no longer possible. I wonder if there is the same "keep around for a short time and then permanently delete" behavior with commits not attached to a branch.

I have two requests:

  1. Could we wait a day and see if that commit is still visible?
  2. Can you check that it's actually possible to checkout to that tag in a local git client (not the GitHub UI)?
    git fetch origin --tags
    git checkout test_tag

@StrikerRUS
Copy link
Collaborator Author

OK, I'll do this.

But please note a yellow note of the screenshot above that indicates that the branch was removed.

@StrikerRUS
Copy link
Collaborator Author

Also, our scenario is described here:

The tag and commit would still exist if the branch is deleted.
https://github.saobby.my.eu.orgmunity/t/what-happens-to-a-tag-on-a-deleted-branch/195995/2

If you delete a branch that a tag was created from, this will have no effect on the tag.
https://stackoverflow.com/a/33283539

@StrikerRUS
Copy link
Collaborator Author

One thing that doesn't work with such type of releases is release auto-changelog.

image

We are lacking all items since v3.3.1. Luckily, I was expecting this and preserved old changelog draft here #4930 (comment). So, we just need manually merge that preserved changelog with new items that will be added since today for v4.0.0 release.

@jameslamb
Copy link
Collaborator

I just uploaded v3.3.2 to CRAN.

image

@shiyu1994 you should receive an email from CRAN asking you to approve this submission.

I included the following note with this submission (will add this to cran-comments.md in a PR)

I am submitting this new version of {lightgbm} on behalf of its maintainer, Yu Shi (yushi2@microsoft.com), with his permission.

This release, v3.3.2, is identical to the existing release on CRAN (v3.3.1) except in one way: it includes the patch at https://svn.r-project.org/R-dev-web/trunk/WindowsBuilds/winutf8/ucrt3/r_packages/patches/CRAN/lightgbm.diff, which Tomas Kalibera and Uwe Ligges emailed us about.

If this submission is approved, please inform Tomas and Uwe so that patch can be removed.

@shiyu1994
Copy link
Collaborator

@jameslamb Upload is now confirmed. Thank you!

@shiyu1994
Copy link
Collaborator

The newly submitted CRAN package did not pass the pretest. Following is the email from CRAN. This seems because CRAN still automatically patches socket_wrapper.hpp and causes compilation error:
https://win-builder.r-project.org/incoming_pretest/lightgbm_3.3.2_20220108_034245/Windows/00install.out
Following is the full text of the email from CRAN.

Dear maintainer,

package lightgbm_3.3.2.tar.gz does not pass the incoming checks automatically, please see the following pre-tests:
Windows: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwin-builder.r-project.org%2Fincoming_pretest%2Flightgbm_3.3.2_20220108_034245%2FWindows%2F00check.log&data=04%7C01%7Cyushi2%40microsoft.com%7C18b33ff99dec4905461308d9d28a30da%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637772314174652416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gunCgJBSltmLhxRc3HrUXEALn9NLCetD4dE7sI5pd7k%3D&reserved=0
Status: 1 ERROR
Debian: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwin-builder.r-project.org%2Fincoming_pretest%2Flightgbm_3.3.2_20220108_034245%2FDebian%2F00check.log&data=04%7C01%7Cyushi2%40microsoft.com%7C18b33ff99dec4905461308d9d28a30da%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637772314174652416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KWmSnTXea%2BnyMVj2hiZu5qzfd8kLh6wSxf7taT0TAuQ%3D&reserved=0
Status: OK

Last released version's CRAN status: OK: 7, NOTE: 6
See: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcran.r-project.org%2Fweb%2Fchecks%2Fcheck_results_lightgbm.html&data=04%7C01%7Cyushi2%40microsoft.com%7C18b33ff99dec4905461308d9d28a30da%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637772314174652416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SH4ek74lElca%2FJBnpUbkNuFv00FHaCNxaPuyLXiX3Q4%3D&reserved=0

CRAN Web: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcran.r-project.org%2Fpackage%3Dlightgbm&data=04%7C01%7Cyushi2%40microsoft.com%7C18b33ff99dec4905461308d9d28a30da%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637772314174652416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=w6RPk93QHKKKGhT9%2FhzDB5A8kRDYtUA%2B2mYbxAGRkZQ%3D&reserved=0

Please fix all problems and resubmit a fixed version via the webform.
If you are not sure how to fix the problems shown, please ask for help on the R-package-devel mailing list:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstat.ethz.ch%2Fmailman%2Flistinfo%2Fr-package-devel&data=04%7C01%7Cyushi2%40microsoft.com%7C18b33ff99dec4905461308d9d28a30da%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637772314174652416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=jtjtXAyY0DEUeCDjzDNsbewXpkW8x%2BG6TAFaXrQQOS0%3D&reserved=0
If you are fairly certain the rejection is a false positive, please reply-all to this message and explain.

More details are given in the directory:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwin-builder.r-project.org%2Fincoming_pretest%2Flightgbm_3.3.2_20220108_034245%2F&data=04%7C01%7Cyushi2%40microsoft.com%7C18b33ff99dec4905461308d9d28a30da%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637772314174652416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=4jAtdHkrwCHCM1pQAEZ6KArlJuCzt8DnT7WDYZpBqBY%3D&reserved=0
The files will be removed after roughly 7 days.

No strong reverse dependencies to be checked.

Best regards,
CRAN teams' auto-check service
00details.log[33215].log

@StrikerRUS
Copy link
Collaborator Author

StrikerRUS commented Jan 9, 2022

This seems because CRAN still automatically patches socket_wrapper.hpp and causes compilation error:

I strongly believe you are right:

** applying installation-time patches
patching file src/network/socket_wrapper.hpp
Hunk #1 FAILED at 60.
Hunk #2 succeeded at 90 with fuzz 1 (offset 4 lines).
1 out of 2 hunks FAILED -- saving rejects to file src/network/socket_wrapper.hpp.rej
WARNING: failed to apply patch patches/CRAN/lightgbm.diff

...

g++  -std=gnu++11 -I"D:/RCompile/recent/R/include" -DNDEBUG -I./include -DEIGEN_MPL2_ONLY -DMM_PREFETCH=1 -DMM_MALLOC=1 -DUSE_SOCKET -DLGB_R_BUILD    -I"d:/rtools42/x86_64-w64-mingw32.static.posix/include"  -fopenmp -pthread   -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign -fexceptions  -c network/linkers_socket.cpp -o network/linkers_socket.o
In file included from network/linkers.h:22,
                 from network/linkers_socket.cpp:20:
network/socket_wrapper.hpp:332:2: error: #endif without #if
  332 | #endif   // LightGBM_NETWORK_SOCKET_WRAPPER_HPP_
      |  ^~~~~
make: *** [D:/RCompile/recent/R/etc/x64/Makeconf:259: network/linkers_socket.o] Error 1
ERROR: compilation failed for package 'lightgbm'

We again need to ask CRAN team somehow to stop patching lightgbm...

@StrikerRUS
Copy link
Collaborator Author

@jameslamb

I have two requests:

  1. Could we wait a day and see if that commit is still visible?
  2. Can you check that it's actually possible to checkout to that tag in a local git client (not the GitHub UI)?
    shell git fetch origin --tags git checkout test_tag

test_branch was removed ~2022-01-07T17:01:42Z. Right now it's 2022-01-09T15:14:12Z.

  1. I still can access target commit at GitHub:
    image
  2. Locally everything is fine as well:
    image

I believe we can remove release_332 branch.

@jameslamb
Copy link
Collaborator

Ok thanks for waiting and checking @StrikerRUS . Let's delete it.

@shiyu1994 can you reply to CRAN and ask them to please stop applying that patch?

@shiyu1994
Copy link
Collaborator

@shiyu1994 can you reply to CRAN and ask them to please stop applying that patch?

Sure. Done with that.

@StrikerRUS StrikerRUS deleted the release_332 branch January 10, 2022 15:30
@StrikerRUS
Copy link
Collaborator Author

@shiyu1994

Sure. Done with that.

Have they answered anything?

On CRAN version is still 3.3.1 but now with r-devel-windows-x86_64-new-UL and r-devel-windows-x86_64-new-TK tests failed with network/socket_wrapper.hpp:241:43: error: call of overloaded 'inet_pton(int, const char*&, in_addr*)' is ambiguous error due to duplicated function definitions.

@shiyu1994
Copy link
Collaborator

They've replied that the automatic patch is removed and checks are retriggered. I just saw the status right now too. I'll send another email to ensure that lightgbm 3.3.2 is being checked.

@StrikerRUS
Copy link
Collaborator Author

I hope they have started new checks:

image

@StrikerRUS
Copy link
Collaborator Author

OK, they have started:

image

Tests on Fedora have passed already.

@StrikerRUS
Copy link
Collaborator Author

OK, all tests have passed! 🎉
https://cran.r-project.org/web/checks/check_results_lightgbm.html

Not sure about additional tests though.

@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 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.