Release request (aka move v1 to v1.7.0)

paulo-ferraz-oliveira commented 3 years ago

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

starbelly commented 3 years ago

@paulo-ferraz-oliveira I agree

starbelly commented 3 years ago

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

starbelly commented 3 years ago

@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 commented 3 years ago

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

starbelly commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@ericmj does not seem to be the case. I had erlef/setup-beam@v1.7.0 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 commented 3 years ago

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 commented 3 years ago

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

paulo-ferraz-oliveira commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

ericmj commented 3 years ago

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 https://github.com/hexpm/hex/pull/873.

starbelly commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/jeremyjh/dialyxir/issues/301 but not 100% sure, as not using force, and only happens after upgrading setup-elixir.

starbelly commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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. https://github.com/brianmay/tp_link_hs100/pull/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 commented 3 years ago

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

paulo-ferraz-oliveira commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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/setup-elixir@v1.7
  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 commented 3 years ago

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

paulo-ferraz-oliveira commented 3 years ago

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 commented 3 years ago

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

brianmay commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

starbelly commented 3 years ago

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

brianmay commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@starbelly, regarding https://github.com/erlef/setup-beam/issues/29#issuecomment-816299477, 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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

brianmay commented 3 years ago

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

starbelly commented 3 years ago

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 commented 3 years ago

Go for it!

paulo-ferraz-oliveira commented 3 years ago

It's been done. Closing.

wojtekmach commented 3 years ago

Congrats and thanks to you and everyone involved!

paulo-ferraz-oliveira commented 3 years ago

Congrats and thanks to you and everyone involved!

I'll celebrate in a month 🤞 haha

erlef / setup-beam

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