search

nim-lang / backport

backport requests
4 stars 1 forks source link

1.6.14 backport candidates #1

Closed arnetheduck closed 1 week ago

arnetheduck commented 1 year ago

Created by:

git log --oneline --pretty="format:%m https://github.com/nim-lang/Nim/commit/%H %s" --left-right  --cherry-pick devel...origin/version-1-6

then filtering by simplicity vs impact

narimiran commented 1 year ago

@ringabout @tersec Did you maybe have some time to go through this list to see if there are things that shouldn't be backported?

I was planning to start with backports tomorrow (in about 24 hours from now), so if you could check the bottom-most (the oldest) 20-ish items on this list, I would really appreciate it.

ringabout commented 1 year ago

@narimiran Btw https://github.com/nim-lang/Nim/commit/4071b3fad8222fb797475d34478f13eb0a99e31a has broken the version-1-6 CI

narimiran commented 1 year ago

@narimiran Btw https://github.com/nim-lang/Nim/commit/4071b3fad8222fb797475d34478f13eb0a99e31a has broken the version-1-6 CI

Ah, my error. I've just pushed a fix.

tersec commented 1 year ago

@narimiran looked through all the remaining ones and they look okay for me. Many are just added test cases, which can't break anything, and only a couple are nontrivial in the compiler per se, but those are also quite useful.

narimiran commented 1 year ago

Many are just added test cases, which can't break anything

That's what I thought too, but..... ;)

To clarify: sometimes it is adding a test for stuff which wasn't backported, so the CIs fail. But I use a "helper branch" to test everything (and remove failing ones) before merging into version-1-6, so the official branch will be kept clean.

arnetheduck commented 1 year ago

I threw the test case PR:s in there because many were written for fixes whose status was "unknown" on the 1.6 branch but they tested things that would be useful to have fixed - if something doesn't work, it would be good to write it down so as to investigate when the fix happened and consider its inclusion in 1.6 as well

narimiran commented 1 year ago

if something doesn't work, it would be good to write it down so as to investigate when the fix happened and consider its inclusion in 1.6 as well

I agree, and my plan was (and after your comment still is) to go through those once I finish going through this list and backport the "easy ones".

ringabout commented 1 year ago

https://github.com/nim-lang/Nim/pull/21448(Allow futureLogging in release builds) needs to be backported too. See also https://github.com/nim-lang/Nim/issues/21758

narimiran commented 1 year ago

https://github.com/nim-lang/Nim/pull/21448 futureLogging in release builds) needs to be backported too. See also https://github.com/nim-lang/Nim/issues/21758

Done

narimiran commented 1 year ago

These are the remaining, currently not backported, commits from the original list:

Originally I didn't backport those because they either added a test for a feature/fix which was not backported, or fixed something that isn't present in the 1.6 line, or similar.

@arnetheduck, are any of the commits in this list a "must have" and should I retry backporting those?

arnetheduck commented 1 year ago

Very nice! Going through the list, here are a few high-value targets remaining - cc @ringabout that was involved in many of them:

Finally, in the category of surprises, it's odd that this fix doesn't compile clean, it's kind of trivial:

narimiran commented 1 year ago

https://github.com/nim-lang/Nim/commit/0bacdf5fdf86a01132d2817599ad0a7f155a101e

This one compiles and runs without an error (when it shouldn't).

https://github.com/nim-lang/Nim/commit/841d9d59755f805245d862456d53e4fd8a426813 This one in particular is very nasty!

It must have been fixed at some point but the issue was not refferrenced in the bugfix, so I didn't find what I need to backport to make this work.

https://github.com/nim-lang/Nim/commit/ecc8f61fe48515ac35360b88d7bb72f76bc7ed68

Running the test produces the following error:

error: ‘T3_’ is a pointer; did you mean to use ‘->’?
  186 |                 T3_.x = (*T4_);
      |                    ^
      |                    ->

https://github.com/nim-lang/Nim/commit/c814c4d993675551ecf388b6a583c471a1b8bc5e

Nim 1.6 doesn't have nfUseDefaultField (which in this PR was changed to nfSkipFieldChecking, and I was reluctant to introduce it.

https://github.com/nim-lang/Nim/commit/16f42084d32144d5afb2a5cc3a5a833e5295a8bc

Same as above.

Finally, in the category of surprises, it's odd that this fix doesn't compile clean, it's kind of trivial:

https://github.com/nim-lang/Nim/commit/705da9d452d19536689a32a8d4378bcce2ec320a

Oh! Now I see it was just a matter of wrong line number in the error message. Fixing now and backporting.

arnetheduck commented 1 year ago

I was reluctant to introduce it.

I think introducing it might be the way to go here - nfUseDefaultField is a new feature in 2.0 so it's natural it didn't exit in the 1.6 branch

narimiran commented 1 year ago

I think introducing it might be the way to go here - nfUseDefaultField is a new feature in 2.0 so it's natural it didn't exit in the 1.6 branch

Ok, I found the PR that introduces it: https://github.com/nim-lang/Nim/pull/20480 I've backported it too, and it is "mostly fine": one test in tests/objects/tobject_default_value.nim fails — the commented out lines below are the ones that are failing:

  block:
    var x: Ref2
    new(x, proc (x: Ref2) {.nimcall.} = discard "call Ref")
    # doAssert x.value == 12, "Ref.value = " & $x.value

    proc call(x: RefInt2) =
      discard "call RefInt"

    var y: RefInt2
    new(y, call)
    # doAssert y.value == 12
    # doAssert y.data == 73

@ringabout, do you maybe recognize this, i.e. is there some other commit/PR that I'm missing which should also be backported for this to work? (The other, very possible, option is that I wrongly resolved merge conflicts)

ringabout commented 1 year ago

I don't think it's a good idea to backport https://github.com/nim-lang/Nim/pull/20480, it's not safe to have it into 1.6.x

narimiran commented 1 year ago

https://github.com/nim-lang/Nim/commit/841d9d59755f805245d862456d53e4fd8a426813 This one in particular is very nasty!

After some bisecting in devel, I managed to find that this bug was fixed in https://github.com/nim-lang/Nim/pull/19972, "defaults to ORC", which is also something that shouldn't be backported, IMO.

arnetheduck commented 1 year ago

I managed to find that this bug was fixed in https://github.com/nim-lang/Nim/pull/19972,

It was not, actually - devel with --refc still has the bug - essentially, it's fixed for orc but not for refc

narimiran commented 1 year ago

It was not, actually - devel with --refc still has the bug - essentially, it's fixed for orc but not for refc

Correct. It was poor wording on my side. I should have said: "....this bug 'disappeared' because of....".

narimiran commented 1 year ago

https://github.com/nim-lang/Nim/commit/c814c4d993675551ecf388b6a583c471a1b8bc5e

Thanks to @ringabout, this is now backported to 1.6!

narimiran commented 1 year ago

...and the previous backport (c814c4d), also allowed https://github.com/nim-lang/Nim/commit/16f42084d32144d5afb2a5cc3a5a833e5295a8bc to be cleanly backported!

arnetheduck commented 1 year ago

a few more :)