Release request (aka move v1 to v1.7.0) #29

paulo-ferraz-oliveira · 2021-04-02T14:22:39Z

Are we ready for v1.7.0? If so, I'm proposing it happens.

starbelly · 2021-04-02T15:11:16Z

starbelly · 2021-04-02T15:20:07Z

@paulo-ferraz-oliveira let's make a 1.7.0 tag, then I will try that out on verl and rebar3_hex

starbelly · 2021-04-02T16:31:55Z

@paulo-ferraz-oliveira While we can create a tag, we should wait until at Monday to do a "release". It's a holiday today for a lot of people.

paulo-ferraz-oliveira · 2021-04-02T20:10:21Z

Sure. Is there any process for tagging? Or is it just pushing the tag on top of master, when the time comes?

starbelly · 2021-04-02T20:11:41Z

@paulo-ferraz-oliveira Just normal tagging. When release time comes we re-write the v1 tag with the latest tag (i.e., v1.7.0)

paulo-ferraz-oliveira · 2021-04-03T01:53:42Z

v1.7.0 and v1.7 are now published and at least 20 pull requests (Erlang + rebar3) show no issues with the current implementation. Will wait for v1 to close this.

ericmj · 2021-04-03T17:12:29Z

It's too late to roll back now but I would have liked to test this more since it's a major rewrite. Remember that there is no such thing as releases for actions, when you push the tag it is released and people can/will start using it.

You don't have to tag the action to test it, you can test the branch or commit ref directly.

starbelly · 2021-04-03T17:28:31Z

@ericmj does not seem to be the case. I had erlef/[email protected] as seen here : https://github.com/jelly-beam/verl/actions/runs/714773107

I just tried changing it to @v1 which still does not work (i.e, the tag has not been re-written yet).

https://github.com/jelly-beam/verl/runs/2260675067?check_suite_focus=true

ericmj · 2021-04-03T17:31:04Z

Tags will of course not change unless you actually change them, but users will see the pushed tag and assume it is stable since 1.7.0 is a stable version.

If we want to test the action there is no reason to tag it since you can use actions without tags.

starbelly · 2021-04-03T17:32:58Z

@ericmj Ahh, I see your point and quite fair.

paulo-ferraz-oliveira · 2021-04-04T10:47:34Z

If we want to test the action there is no reason to tag it since you can use actions without tags.

I assumed it was enough tested (I'd tested in in 20 private repo.s, for example); to be clear, I didn't tag it for testing purposes, but to actually use it elsewhere (as stable).

paulo-ferraz-oliveira · 2021-04-04T10:57:17Z

The tests go like:

download this Erlang + that Elixir and run this mix test
download this Erlang + that rebar3 and run this rebar3 test
(and this is done in a combination of versions, already)

Since we're testing the action itself, not Erlang, Elixir, mix or rebar, the only real tests I can imagine adding are more around obtaining the versions from the listing. What did you have in mind?

starbelly · 2021-04-06T17:47:26Z

Per recent changes we would be best to wait a few weeks and do moar testing before cutting on v1 IMO.

ericmj · 2021-04-06T19:52:42Z

I assumed it was enough tested (I'd tested in in 20 private repo.s, for example); to be clear, I didn't tag it for testing purposes, but to actually use it elsewhere (as stable).

That's great, I didn't realize you had done such extensive testing (sorry if I missed you saying that). Did you also test the existing functionality with just Elixir? I think that's the most important to test since it can break existing users, unlike the new Erlang support which only have new users trying it.

I am currently testing it on hexpm/hex#873.

starbelly · 2021-04-06T19:58:11Z

@ericmj I did testing with elixir via erlef/website, but not easy to test combos with that per deps, but I felt good about it. That said, I think it would still be good to wait a few so we can be sure and get in any other small changes that we see as needed. I believe @paulo-ferraz-oliveira is about to add more tests around OTP 19 and ubuntu as an example.

paulo-ferraz-oliveira · 2021-04-06T20:06:44Z

Yes, I'm PRing in order to close #28. Don't mind waiting for as much as you guys feel comfortable with. I was also thinking that the tests should use all supported Ubuntu versions, 16, 18 and 20, which is not the case (only 18 at the moment). @ericmj, unfortunately I tested only Erlang and rebar3, not Elixir, so understand we need to test this further (only real test was a somewhat big matrix that @starbelly prepared once and we kept updating the action until it all passed).

brianmay · 2021-04-08T02:43:44Z

I am seeing dialyzer errors after upgrading to 1.7 - can submit bug report if required. https://github.com/brianmay/tp_link_hs100/runs/2293025513. Might be related to jeremyjh/dialyxir#301 but not 100% sure, as not using force, and only happens after upgrading setup-elixir.

starbelly · 2021-04-08T18:37:18Z

@brianmay I usually see this error when I built PLTs for version X, but then tried to use them using version Y. This might allude to bugs in previous versions vs a bug in 1.7.0, but I can not say for certain. Definitely appreciate you commenting here though! ❤️

paulo-ferraz-oliveira · 2021-04-08T21:23:18Z

@brianmay, is the only difference between what you had (with no dialyzer issues) and what you have now (with dialyzer issues) the action version bump?

Edit: ... namely, what I'd like to compare is the downloaded elements (Elixir and Erlang/OTP) version-wise, since that's where the difference in behaviour will most likely reside, if the answer to the previous is "yes".

brianmay · 2021-04-08T22:09:48Z

@paulo-ferraz-oliveira Yes, that is correct. This is a dependabot pull request that is failing, that only makes that change. No changes to erlang versions, elixir versions, or anything else. Without the change, master is fine. brianmay/tp_link_hs100#19

I have made to invalidate the PLT cache. Will be interesting to see what happens. Currently rebudiling the PLTs. My understanding is that upgrading the action should not invalidate the cached data however,

brianmay · 2021-04-08T22:11:57Z

OK, recreating the PLT cache appeared to work... Go figure...

paulo-ferraz-oliveira · 2021-04-08T22:17:16Z

The reason I asked about the versions, specifically, is because the plugin potentially changes the downloaded ones (by "trying to guess" - which it probably doesn't need to). While previously you might have been downloading v1.11.3 you are now most surely downloading v1.11.3-otp-23 and that might justify the change 🤷‍♂️. I'm OK to revert this change, if there's no further impact. @ericmj?

Also, AFAIK the cached PLT should be compatible for that one small change, yes, but also I'm not exactly sure what this change is for.

brianmay · 2021-04-08T22:19:42Z

Not sure why the confusion. There are two changes here. One bumps the version of action, and the other one creates a new PLT cache.

In both cases I have said to use Erlang 23.2.7 and Elixir 1.11.3 - that has not changed.

starbelly · 2021-04-08T22:37:35Z

@brianmay @paulo-ferraz-oliveira here is my hunch. And @paulo-ferraz-oliveira can answer this better than me, but iirc we found a bug where the wrong OTP version might have been brought in prior to 1.7.0. But let's hear what @paulo-ferraz-oliveira says since he did 99% of the work.

brianmay · 2021-04-08T22:46:58Z

@starbelly Oh, OK, I get you now. The logs do give the correct version. But depending on where the bug is, that may not matter:

Run erlef/setup-elixir@v1
  with:
    elixir-version: 1.11.3
    otp-version: 23.2.7
    install-hex: true
    install-rebar: true
Installing OTP OTP-23.2.7 - built on ubuntu-20.04
  /home/runner/work/_actions/erlef/setup-elixir/v1/dist/install-otp OTP-23.2.7 ubuntu-20.04
  
Installing Elixir v1.11.3
  /home/runner/work/_actions/erlef/setup-elixir/v1/dist/install-elixir v1.11.3 
  Archive:  v1.11.3.zip
[...]

After the update I get:

Run erlef/[email protected]
  with:
    elixir-version: 1.11.3
    otp-version: 23.2.7
    install-hex: true
    install-rebar: true
  env:
    MIX_ENV: test
Installing Erlang/OTP OTP-23.2.7 - built on ubuntu-20.04
  /home/runner/work/_actions/erlef/setup-elixir/v1.7/dist/install-otp ubuntu-20.04 OTP-23.2.7
  
  Installed Erlang/OTP version follows
  Erlang (SMP,ASYNC_THREADS,HIPE) (BEAM) emulator version 11.1.8
Using Elixir / -otp- version v1.11.3-otp-23
Installing Elixir v1.11.3-otp-23
  /home/runner/work/_actions/erlef/setup-elixir/v1.7/dist/install-elixir v1.11.3-otp-23
  Installed Elixir version follows
  IEx 1.11.3 (compiled with Erlang/OTP 23)

Not 100% sure what I am looking at here, but guessing that while it installed the right erlang, the old version didn't install the right elixir.

starbelly · 2021-04-08T22:48:56Z

@brianmay Hmm yeah that invalidates my hunch. Thank you for sharing that.

paulo-ferraz-oliveira · 2021-04-08T22:52:05Z

the old version didn't install the right elixir.

I wouldn't say it was "wrong", since it was working (and the Elixir version, by itself, was as expected), but it wasn't the one built with the pull Erlang/OTP version (which is now the case). This was the reason why I wrote @ericmj, because if there's no reason to add -otp-... in v1.7.0 (at the expense of potentially invalidating PLTs and the like) we might decide to go back on that decision.

starbelly · 2021-04-08T22:57:11Z

@paulo-ferraz-oliveira I see. Requires testing.

brianmay · 2021-04-08T22:58:11Z

Not sure I fully understand the difference between 1.11.3 and 1.11.3-otp-23. I think I would expect to get the later. But maybe make it a config option that is disabled by default, meaning you get the old behaviour by default?

paulo-ferraz-oliveira · 2021-04-08T23:15:04Z

@brianmay, I'm not sure we want to add an option to get the old behaviour; we might either decide what you reported leads to a necessary consumption change (and we keep as-is - i.e. the consumer adapts) or it doesn't (and we revert to previous behaviour - the producer adapts). I'm open to all options, but we have to decide first.

Also:

v1.11.3: Elixir compiled with Erlang/OTP 21 (equivalent to v1.11.3-otp-21)
v1.11.3-otp-23: Elixir compiled with Erlang/OTP 23

brianmay · 2021-04-08T23:20:49Z

I am happy either way. But ideally we should stick to any decision (unless we have a really good reason to change).

starbelly · 2021-04-08T23:22:09Z

Replicated! : https://github.com/starbelly/test-setup-elixir/runs/2301526919?check_suite_focus=true

brianmay · 2021-04-08T23:23:59Z

Thinking about it a bit more, I tend to think v1.11.3-otp-23 holds least surprise. i.e. it is what I would expect without reading documentation or source code. So I would tend to maybe prefer that version. Even if it does mean having to fix my CI builds.

starbelly · 2021-04-08T23:29:31Z

Yeah, so my personal take is thus :

I like how the behavior tries to do what you want that is... Instead of pulling in elixir 1.x build with OTP say 21, it will not do it's best to bring in elixir 1.x built with OTP 23 (in the case you're specifying this).
It's going to be no fun for some to bust their cache, but I can think of worse things. In addition, if we make it clear (via this issue or release notes) this is why you might run into this, then I think most people will be fine with that and happy with the change.

Need to hear from both @paulo-ferraz-oliveira and @ericmj ofc.

paulo-ferraz-oliveira · 2021-04-08T23:33:30Z

I am happy either way. But ideally we should stick to any decision (unless we have a really good reason to change).

I saw the previous change as a bug fix, so I stuck to that decision (it was my really good reason to change). 😄

paulo-ferraz-oliveira · 2021-04-08T23:35:43Z

@starbelly, regarding #29 (comment), I'm OK to keep stuff as-is (I see it as the path of least surprise). I'd like to have @ericmj pitch in too, though.

ericmj · 2021-04-09T08:13:26Z

Can you explain the dialyzer issue? It looks like doesn't find hipe-4.0.1/ebin/erl_bif_types.beam, how did we come to the conclusion that it is related to how we now select the correct precompiled Elixir?

EDIT: I think the issue is caused by the path change from .setup-elixir to .setup-beam.

starbelly · 2021-04-09T14:42:31Z

@ericmj oooh, that's possible. I hope that's not that case as that implies it should have been version 2.

EDIT:

Hmm, no I don't believe that makes sense. PLT information should go into ~/.mix and per this case in priv/plts/ as well AFAIK.

I've run into this issue multiple times in the past, but locally, when switching from one OTP version to another and i conjunction with elixir. And it's always the signal for me that I have PLTs compiled with another version that are not compat with what I'm using at the time.

Of course, I'd say in this issue, it is has not been fully quantified.

ericmj · 2021-04-09T21:58:21Z

Do you have better explanation for the issue? The error says it cannot find a file in .setup-elixir/otp/... and we have changed the path to .setup-beam/otp so it seems likely the issue is that dialyzer can no longer find the files it based the PLT on. It could be argued that dialyzer should bust the cache instead of erroring out in that case, but it doesn't.

If this is an important issue we can revert the paths back to .setup-elixir since they are internal only.

starbelly · 2021-04-09T22:08:15Z

I see. I missed that @ericmj. Appreciate your hawk eyes. So yes, that seems to very much be the issue. Thus, we should probably add a section in the readme, thoughts?

paulo-ferraz-oliveira · 2021-04-10T13:28:27Z

how did we come to the conclusion that it is related to how we now select the correct precompiled Elixir?

I've not come to a conclusion, but some speculation 😄 which is why I referred to you (@ericmj), explicitly, four times.

If this is an important issue we can revert the paths back to .setup-elixir since they are internal only.

I'm OK with this change. But I'm not sure on "is an important issue". For me it's not, but I'm not the only one judging it.

If we do that change, though (the folder name) supposedly we wouldn't need what @starbelly proposes (a change to the README). I'll let you guys decide.

My votes go for:

it's kept unchanged,
no notes in README,
if an issue surfaces we steer developers to this one and tell them the cache should be busted.

brianmay · 2021-04-13T23:06:17Z

So, in summary, I believe everyone agrees the current bahaviour should be kept unchanged, right?

I do believe this should be clearly documented. Under release notes or something similar. Nothing more annoying then spending ages debugging a known problem. If an issue arises and the developer contacts this project, then yes, they can get steered in the right direction. But, the nature of this problem makes it look like a dialyzer problem, not a problem with the action that successfully completed earlier. Dialyzer maintainers, dialyzer support groups, and general elixir support groups will probably get consulted first, and these groups cannot help (In fact I did that). Maybe, just maybe, adding something to the release notes could help somebody work out the problem faster without going through all that.

starbelly · 2021-04-13T23:21:08Z

I agree with:

Don't change it
Document in release notes and/or README (Paulo not in favor of README)

To be clear.

Input from @ericmj needed.

paulo-ferraz-oliveira · 2021-04-14T00:49:01Z

I went ahead and added this to the release notes. Let me know if any of it has to be changed.

brianmay · 2021-04-14T05:12:44Z

Looks good to me. I guess I should fix my builds now :-)

starbelly · 2021-04-19T22:03:01Z

Ok, so I think we've waited long enough and we should re-write @v1 at this point. I start a new job in a week and I'd rather cut this now, have to respond to some issues this week vs next.

I know @paulo-ferraz-oliveira is ready, @ericmj ?

ericmj · 2021-04-19T22:05:19Z

Go for it!

paulo-ferraz-oliveira · 2021-04-19T22:18:44Z

It's been done. Closing.

wojtekmach · 2021-04-19T22:49:53Z

Congrats and thanks to you and everyone involved!

paulo-ferraz-oliveira · 2021-04-19T22:51:20Z

Congrats and thanks to you and everyone involved!

I'll celebrate in a month 🤞 haha

paulo-ferraz-oliveira changed the title ~~Release request~~ Release request (aka move v1 to v1.7.0) Apr 3, 2021

paulo-ferraz-oliveira closed this as completed Apr 19, 2021

Release request (aka move v1 to v1.7.0) #29

Release request (aka move v1 to v1.7.0) #29

Comments

paulo-ferraz-oliveira commented Apr 2, 2021

starbelly commented Apr 2, 2021

starbelly commented Apr 2, 2021

starbelly commented Apr 2, 2021 • edited Loading

paulo-ferraz-oliveira commented Apr 2, 2021

starbelly commented Apr 2, 2021 • edited Loading

paulo-ferraz-oliveira commented Apr 3, 2021

ericmj commented Apr 3, 2021

starbelly commented Apr 3, 2021 • edited Loading

ericmj commented Apr 3, 2021 • edited Loading

starbelly commented Apr 3, 2021

paulo-ferraz-oliveira commented Apr 4, 2021 • edited Loading

paulo-ferraz-oliveira commented Apr 4, 2021

starbelly commented Apr 6, 2021

ericmj commented Apr 6, 2021

starbelly commented Apr 6, 2021

paulo-ferraz-oliveira commented Apr 6, 2021

brianmay commented Apr 8, 2021 • edited Loading

starbelly commented Apr 8, 2021

paulo-ferraz-oliveira commented Apr 8, 2021 • edited Loading

brianmay commented Apr 8, 2021

brianmay commented Apr 8, 2021

paulo-ferraz-oliveira commented Apr 8, 2021

brianmay commented Apr 8, 2021 • edited Loading

starbelly commented Apr 8, 2021

brianmay commented Apr 8, 2021 • edited Loading

starbelly commented Apr 8, 2021

paulo-ferraz-oliveira commented Apr 8, 2021 • edited Loading

starbelly commented Apr 8, 2021

brianmay commented Apr 8, 2021

paulo-ferraz-oliveira commented Apr 8, 2021 • edited Loading

brianmay commented Apr 8, 2021

starbelly commented Apr 8, 2021

brianmay commented Apr 8, 2021 • edited Loading

starbelly commented Apr 8, 2021 • edited Loading

paulo-ferraz-oliveira commented Apr 8, 2021

paulo-ferraz-oliveira commented Apr 8, 2021

ericmj commented Apr 9, 2021 • edited Loading

starbelly commented Apr 9, 2021 • edited Loading

ericmj commented Apr 9, 2021

starbelly commented Apr 9, 2021

paulo-ferraz-oliveira commented Apr 10, 2021

brianmay commented Apr 13, 2021

starbelly commented Apr 13, 2021

paulo-ferraz-oliveira commented Apr 14, 2021

brianmay commented Apr 14, 2021

starbelly commented Apr 19, 2021

ericmj commented Apr 19, 2021

paulo-ferraz-oliveira commented Apr 19, 2021

wojtekmach commented Apr 19, 2021 • edited Loading

paulo-ferraz-oliveira commented Apr 19, 2021

starbelly commented Apr 2, 2021 •

edited

Loading

starbelly commented Apr 2, 2021 •

edited

Loading

starbelly commented Apr 3, 2021 •

edited

Loading

ericmj commented Apr 3, 2021 •

edited

Loading

paulo-ferraz-oliveira commented Apr 4, 2021 •

edited

Loading

brianmay commented Apr 8, 2021 •

edited

Loading

paulo-ferraz-oliveira commented Apr 8, 2021 •

edited

Loading

brianmay commented Apr 8, 2021 •

edited

Loading

brianmay commented Apr 8, 2021 •

edited

Loading

paulo-ferraz-oliveira commented Apr 8, 2021 •

edited

Loading

paulo-ferraz-oliveira commented Apr 8, 2021 •

edited

Loading

brianmay commented Apr 8, 2021 •

edited

Loading

starbelly commented Apr 8, 2021 •

edited

Loading

ericmj commented Apr 9, 2021 •

edited

Loading

starbelly commented Apr 9, 2021 •

edited

Loading

wojtekmach commented Apr 19, 2021 •

edited

Loading