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.

Sign up for GitHub

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

Jump to bottom

1.6.14 backport candidates #1

Closed
arnetheduck opened this issue Apr 19, 2023 · 21 comments
Closed

1.6.14 backport candidates #1

arnetheduck opened this issue Apr 19, 2023 · 21 comments

Comments

@arnetheduck
Copy link

arnetheduck commented Apr 19, 2023 •
edited by narimiran
Loading

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
Copy link
Member

@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
Copy link
Member

@narimiran Btw nim-lang/Nim@4071b3f has broken the version-1-6 CI

@narimiran
Copy link
Member

@narimiran Btw nim-lang/Nim@4071b3f has broken the version-1-6 CI

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

@tersec
Copy link

tersec commented Apr 26, 2023

@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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

nim-lang/Nim#21448 futureLogging in release builds) needs to be backported too. See also nim-lang/Nim#21758

@ringabout ringabout mentioned this issue May 2, 2023
Closed
@narimiran
Copy link
Member

nim-lang/Nim#21448 futureLogging in release builds) needs to be backported too. See also nim-lang/Nim#21758

Done

@narimiran
Copy link
Member

narimiran commented May 8, 2023 •
edited
Loading

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
Copy link
Author

arnetheduck commented May 23, 2023 •
edited by narimiran
Loading

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
Copy link
Member

narimiran commented May 24, 2023 •
edited
Loading

nim-lang/Nim@0bacdf5

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

nim-lang/Nim@841d9d5 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.

nim-lang/Nim@ecc8f61

Running the test produces the following error:

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

nim-lang/Nim@c814c4d

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

nim-lang/Nim@16f4208

Same as above.

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

nim-lang/Nim@705da9d

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

@arnetheduck
Copy link
Author

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
Copy link
Member

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: nim-lang/Nim#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
Copy link
Member

ringabout commented May 29, 2023 •
edited
Loading

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

@narimiran
Copy link
Member

nim-lang/Nim@841d9d5 This one in particular is very nasty!

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

@arnetheduck
Copy link
Author

I managed to find that this bug was fixed in nim-lang/Nim#19972,

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

@narimiran
Copy link
Member

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
Copy link
Member

nim-lang/Nim@c814c4d

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

@narimiran
Copy link
Member

...and the previous backport (c814c4d), also allowed nim-lang/Nim@16f4208 to be cleanly backported!

@arnetheduck
Copy link
Author

arnetheduck commented Jun 15, 2023 •
edited by narimiran
Loading

a few more :)

@arnetheduck arnetheduck changed the title 1.6 backport candidates 1.6.14 backport candidates Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@arnetheduck @tersec @narimiran @ringabout