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

Backports release 1.5.3 #38122

Merged
merged 15 commits into from
Nov 7, 2020
Merged

Backports release 1.5.3 #38122

merged 15 commits into from
Nov 7, 2020

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Oct 21, 2020

Backported PRs:

Need manual backport:

Contains multiple commits, manual intervention needed:

KristofferC and others added 4 commits October 21, 2020 08:02
add an error check that the max size is not exceeded

(cherry picked from commit 5f55b97)
This reverts commit cf0c3e0.

(cherry picked from commit b18647e)
@KristofferC KristofferC added the release Release management and versioning. label Oct 21, 2020
@yuyichao
Copy link
Contributor

#37557 introduced a bug that is fixed by #37896

@Sacha0
Copy link
Member

Sacha0 commented Oct 21, 2020

@KristofferC, I hope you don't mind: I took the liberty of pushing the patches we carry for

#37594 - dump: ensure Array eltype layout is initialized early
#37101 - make Profile more thread/signal-safe
#36821 - Fix function call to rec_backtrace_ctx when using USE_SYSTEM_LIBUNWIND=1

on Julia 1.5.2 and checked off / added those to your list of manual backports at top :). (Those patches have been exercised by our team for a few weeks each now.)

@Sacha0
Copy link
Member

Sacha0 commented Oct 22, 2020

Hm, would #38049 be a backport candidate once it merges, what with fixing precompilation failures?

@KristofferC
Copy link
Member Author

Seems like a nice bug fix, so makes sense to me. Usually it is good to have PRs be on master for a while to shake out anything egregiously wrong but that should be pretty harmless and easy to test.

@KristofferC
Copy link
Member Author

@Sacha0, seems like Windows is not happy with one of the backports?

@martinholters
Copy link
Member

Include #37769?

@dkarrasch
Copy link
Member

Also include #38002?

@Sacha0
Copy link
Member

Sacha0 commented Oct 22, 2020

@Sacha0, seems like Windows is not happy with one of the backports?

Hm, I don't think we have anyone running these patches on Windows, so perhaps that's not entirely surprising 😄. I suspect the patch for #37101; having a look at the logs now.

@Sacha0
Copy link
Member

Sacha0 commented Oct 22, 2020

Yes, it looks like #37101 introduces the failure in package_win{32,64}. Amusingly the failure is the Windows-equivalent of #36821:

In file included from /cygdrive/c/buildbot/worker/package_win64/build/src/signal-handling.c:117:
/cygdrive/c/buildbot/worker/package_win64/build/src/signals-win.c:354:36: error: too few arguments to function 'rec_backtrace_ctx'
  354 |                     bt_size_cur += rec_backtrace_ctx((jl_bt_element_t*)bt_data_prof + bt_size_cur,
      |                                    ^~~~~~~~~~~~~~~~~

i.e. the rec_backtrace_ctx calls introduced by #37101 need the trailing argument that #36821 adds to a call in signals-mach.c.

@Sacha0
Copy link
Member

Sacha0 commented Oct 22, 2020

I've pushed a commit that should sort out the error identified above. I'm not confident that fix will sort Windows out altogether, but let's ask the buildbot 🤞 :).

@Sacha0
Copy link
Member

Sacha0 commented Oct 22, 2020

It looks like package_win32 failed in a new and exciting way also related to #37101 (particularly its removal of jl_trygetUnwindInfo). Having a look now :).

@Sacha0 Sacha0 force-pushed the backports-release-1.5 branch from fd03cb5 to a408c1d Compare October 22, 2020 23:59
@Sacha0
Copy link
Member

Sacha0 commented Oct 23, 2020

That last push seems to have fixed the package_ failures, but surfaced another new and exciting failure in tester_win32. I suspect that fixing the latter requires adding #37002 to the patch. I will give that a whirl tomorrow.

Add an `unwrapva` to ensure the returned type is not a `Vararg`.

(cherry picked from commit 22b5d93)
This should make this previously non-atomic mutex threadsafe.

Fixes #35117
@Sacha0 Sacha0 force-pushed the backports-release-1.5 branch from a408c1d to bb83a29 Compare October 23, 2020 21:58
@Sacha0
Copy link
Member

Sacha0 commented Oct 23, 2020

I think I've managed to backport #37002 with #37101 on top, by which I mean the result appears to function locally. Let's see what the buildbot thinks? :)

@Sacha0
Copy link
Member

Sacha0 commented Oct 23, 2020

(I should add that #37002 should obviate the patch from #36821, so I dropped the latter.)

@Sacha0
Copy link
Member

Sacha0 commented Oct 24, 2020

The package_linuxaarch64 failure was a timeout, and the logs look otherwise clean. So we seem to be in good shape. I'll stop messing with your branch now @KristofferC 😄.

@KristofferC
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":release-1.5")

@KristofferC

This comment has been minimized.

@nanosoldier

This comment has been minimized.

@christopher-dG

This comment has been minimized.

@KristofferC

This comment has been minimized.

@nanosoldier

This comment has been minimized.

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@christopher-dG

This comment has been minimized.

@christopher-dG

This comment has been minimized.

@NHDaly
Copy link
Member

NHDaly commented Oct 26, 2020

Hm, would #38049 be a backport candidate once it merges, what with fixing precompilation failures?

Seems like a nice bug fix, so makes sense to me. Usually it is good to have PRs be on master for a while to shake out anything egregiously wrong but that should be pretty harmless and easy to test.

It sounds like we're planning to backport this? I've opened a PR against this branch, here:
#38183

@KristofferC
Copy link
Member Author

@nanosoldier runtests(["Autologistic", "BlobTracking", "Cambrian", "Catalyst", "ChainRules", "ClimateERA", "CodeTransformation", "DiffEqOperators", "FlashWeave", "ImageDistances", "Oscar", "RiskAdjustedLinearizations", "Sched", "StochasticDelayDiffEq", "Tar", "Thorn", "TimeToLive", "Turmeric"], vs = ":release-1.5")

@KristofferC
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs = ":release-1.5")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG

@KristofferC
Copy link
Member Author

https://github.com/perrutquist/CodeTransformation.jl segfaults but that one heavily uses nternals so not sure we can do much about that.

@Sacha0
Copy link
Member

Sacha0 commented Nov 7, 2020

From a brief review of this thread, it seems like this pull request, and ergo 1.5.3, is ready to go? Is merge etc waiting on anything at this point? :)

@KristofferC
Copy link
Member Author

I think this should be good to go from my pov as well.

@KristofferC KristofferC merged commit 006853a into release-1.5 Nov 7, 2020
@KristofferC KristofferC deleted the backports-release-1.5 branch November 7, 2020 15:28
@dkarrasch
Copy link
Member

What about #38122 (comment)? It fixes something that got broken in v1.5.0.

@racinmat
Copy link

Hi, is there a chance #37243 will make it to some 1.5.* release? It seems it didn't make it to 1.5.2 nor 1.5.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Release management and versioning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.