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

Prevent downstream (consumer lib.) warnings #121

Merged
merged 12 commits into from
May 20, 2021
Merged

Prevent downstream (consumer lib.) warnings #121

merged 12 commits into from
May 20, 2021

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

This is "part 2" of #120, where I only bump gpb, regenerate the proto-code files and lock to a specific gpb version, as per comments in that pull request.

rebar.config Outdated Show resolved Hide resolved
@paulo-ferraz-oliveira
Copy link
Contributor Author

I'm bumping the rebar3 patch version, only, since going back to OTP 17 is a stretch for it. I'm not sure it'll work (let's see what CI says).

@wojtekmach
Copy link
Member

Its fine if we can only regenerate pb modules on latest rebar, we only need to do it once and we have ci to ensure the generated Erlang code works on old OTPs.

@paulo-ferraz-oliveira
Copy link
Contributor Author

Its fine if we can only regenerate pb modules on latest rebar, we only need to do it once and we have ci to ensure the generated Erlang code works on old OTPs.

Sure, but CI failing on rebar3 (e.g. for a failure that leads to calling :get_stacktrace) will be difficult to debug pre-22.

@paulo-ferraz-oliveira
Copy link
Contributor Author

The plugin also pulls gpb: https://github.com/lrascao/rebar3_gpb_plugin/blob/develop/rebar.config#L15. Do we still want to lock it? I ask because the files will only be explicitly generated with rebar3 as dev compile and not everytime hex_core is compiled.

@wojtekmach
Copy link
Member

Why would debugging be problematic pre-22? What do you propose instead?

gpb should be a dev dependency, its only needed for hex_core development, it shouldnt be a runtime dep for downstream packages.

@wojtekmach
Copy link
Member

wojtekmach commented May 20, 2021

Yes I would like to lock it still. You are right that the files would be regenerated under specific circumstances but id like to avoid a situation where we have unnecessary churn in them because someone happened to pull in a gpb version that was more recent. I think it is important to have precise control of the version and bump it explicitly not accidentally.

@paulo-ferraz-oliveira
Copy link
Contributor Author

I think it is important to have precise control of the version and bump it explicitly not accidentally.

I agree, I also like to use precise versions in most circumstance. But since this wasn't being done, and I knew not the history behind it... 😄 I'm rebasing the initial change to then show the update to 4.17.5 after rm.

@paulo-ferraz-oliveira
Copy link
Contributor Author

Why would debugging be problematic pre-22?

Because 3.16 contains code that won't run pre-22, most surely, (at least pre-21) like the new :Stacktrace syntax and other OTP changes (mind you, it is compiled with 22).

What do you propose instead?

Maybe lock other rebar3 versions (per older OTP version) and hope for the best (?) You can also think about bumping from 17, but that's your call.

gpb should be a dev dependency, its only needed for hex_core development, it shouldnt be a runtime dep for downstream packages.

Sure. If you use project_plugins it'll never be considered by downstream. The final version of this PR should not contain deps, but in that case rebar.lock'll be empty, too.

@wojtekmach
Copy link
Member

Maybe lock other rebar3 versions (per older OTP version) and hope for the best (?) You can also think about bumping from 17, but that's your call.

Lock where? Sorry, I'm totally lost, I must be missing something. My understanding is hex_core codebase is mostly independent of the rebar3 version. The dependant bits are I suppose the contents of rebar.config and possibly which plugins we use. An important plugin we use is the rebar3_gpb and if it turns out the version we pin does not work on older rebar3 releases, I'm fine with that. It means we won't be able to do e.g. asdf local rebar 3.6 && asdf local erlang 17 && rebar3 as dev compile but I think that's fine with me as well. As long as we can run tests on older OTPs and rebars, that's good enough for me.

I agree, I also like to use precise versions in most circumstance. But since this wasn't being done, and I knew not the history behind it...

We locked the gpb version but only like 2 weeks ago :D

@paulo-ferraz-oliveira
Copy link
Contributor Author

Okay, yeah, naming and context is important, sorry.

So, when I say "lock the gpb version" I don't mean "have it explicit in rebar.config"; I mean "have it locked in rebar.lock".
But you don't want that, since you don't want consumers of hex_core to download a dev. dependency. Fair enough: that won't be in the final version of the pull request.

When I say "lock the rebar3 version" I mean "lock it in the GHA file". We might want that if using latest on older Erlang versions fails (which most surely will if you use a OTP 17 / rebar3 3.16.0 combo, for example).

As long as we can run tests on older OTPs and rebars, that's good enough for me.

And that's what I want to keep also, but that might (as explained) need to force a rebar3 lock in the GHA yml file. It's doable.

We locked the gpb version but only like 2 weeks ago :D

Got it. At the same time, when you say "locked" you mean you "made it explicit" (as you did, in the dev profile). I'm not sure rebar3 will always honour non-rebar3.lock versions, especially when dealing with plugins, but I'll be sure to test this as much as possible.

@paulo-ferraz-oliveira
Copy link
Contributor Author

And now for the fun part. It seems gpb 4.17.5 introduced some stuff regarding versioning that I don't know yet (so will have to). In the meantime I'm push a not-yet-final version of stuff, so you can see how the updates (per commit) would happen in the future.

@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as draft May 20, 2021 12:12
@paulo-ferraz-oliveira
Copy link
Contributor Author

Converted to draft since it's ongoing and might change a bit...

@wojtekmach
Copy link
Member

At the same time, when you say "locked" you mean you "made it explicit" (as you did, in the dev profile).

Yeah, sorry about any confusion. My understanding is rebar.lock of this project is ignored in downstream projects so if we can lock the version in, well, rebar.lock, that's perfect. I just couldn't make it work locally for some reason, probably something to do with my setup. If you were able to verify it works as expected that's perfect, I'm keen on having a proper solution for this.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented May 20, 2021

in, well, rebar.lock, that's perfect

Nah, if you lock it there, dependants will pull it, for no reason. There isn't a concept of development dependencies yet 😄

I'm struggling with the newer version of gpb (inside the dev profile) since it seems there's a new option for .vsn and stuff and... still reading!

Note: tomas-abrahamsson/gpb#207 opened, to understand what's up with 4.17.5+, since we need 4.17.6.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented May 20, 2021

Apart from the failing CI, this is rebar.config form I think is Okay.

Edit: actually, CI not failing any more.

@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as ready for review May 20, 2021 13:49
@paulo-ferraz-oliveira
Copy link
Contributor Author

@wojtekmach, let me know if anything else is required. I'm still going to test this with rebar3_hex.

@wojtekmach
Copy link
Member

Oh, it just worked as a dep after all? Sorry for wasting your time. Looks good to me, lmk after you've done your tests and I'll merge this. Thanks!

@paulo-ferraz-oliveira
Copy link
Contributor Author

Oh, it just worked as a dep after all? Sorry for wasting your time. Looks good to me, lmk after you've done your tests and I'll merge this. Thanks!

No time wasted, no worries. rebar3_hex seems to eat this version right, so if you want you can merge. I'll have to CI that and check if nothing breaks, but static analysis (to start with) is Okay.

@wojtekmach
Copy link
Member

wojtekmach commented May 20, 2021

I tried downgrading to 4.17.0 and re-creating modules and I'm seeing they were generated with 4.17.6 (in other words, they haven't changed at all). What am I doing wrong?

~/src/hex_core[feature/gpb-update]% rebar3 version
rebar 3.15.1 on Erlang/OTP 24 Erts 12.0
~/src/hex_core[feature/gpb-update]% rebar3 plugins list
--- Global plugins ---
rebar3_hex (6.11.3)

~/src/hex_core[feature/gpb-update]% gs
On branch feature/gpb-update
nothing to commit, working tree clean
~/src/hex_core[feature/gpb-update]% sed -i '' s/4.17.6/4.17.0/ rebar.config
~/src/hex_core[feature/gpb-update]% gd
diff --git a/rebar.config b/rebar.config
index 086be6e..4b1b0c8 100644
--- a/rebar.config
+++ b/rebar.config
@@ -21,7 +21,7 @@
 {profiles, [
     {dev, [
         {deps, [
-            {gpb, "4.17.6"}
+            {gpb, "4.17.0"}
         ]},
         {plugins, [
             {rebar3_gpb_plugin, "2.20.1"}
~/src/hex_core[feature/gpb-update]% rm -rf src/hex_pb_* ~/.cache/rebar3 _build
~/src/hex_core[feature/gpb-update]% gs
On branch feature/gpb-update
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   rebar.config
	deleted:    src/hex_pb_names.erl
	deleted:    src/hex_pb_package.erl
	deleted:    src/hex_pb_signed.erl
	deleted:    src/hex_pb_versions.erl

no changes added to commit (use "git add" and/or "git commit -a")
~/src/hex_core[feature/gpb-update]% rebar3 as dev compile
=WARNING REPORT==== 20-May-2021::16:25:07.386244 ===
Description: "Authenticity is not established by certificate path validation"
     Reason: "Option {verify, verify_peer} and cacertfile/cacerts is missing"

===> Fetching rebar3_hex v6.11.3
===> Fetching hex_core v0.7.1
===> Fetching verl v1.0.2
===> Analyzing applications...
===> Compiling verl
===> Compiling hex_core
===> Compiling rebar3_hex
../.cache/rebar3/plugins/rebar3_hex/src/rebar3_hex_repo.erl:128:32: Warning: public_key:ssh_encode/2 is deprecated and will be removed in OTP 26; use ssh_file:encode/2 instead

===> Fetching rebar3_gpb_plugin v2.20.1
===> Fetching gpb v4.17.6
===> Analyzing applications...
===> Compiling gpb
===> Compiling rebar3_gpb_plugin
Compiling descriptor.proto...
Compiling gpb_descriptor.erl...
Compiling gpb_compile_descr.proto...
===> Verifying dependencies...
===> Fetching gpb v4.17.0
===> Analyzing applications...
===> Compiling gpb
Compiling descriptor.proto...
Compiling gpb_descriptor.erl...
Compiling gpb_compile_descr.proto...
===> Analyzing applications...
===> Compiling hex_core
~/src/hex_core[feature/gpb-update]% gs
On branch feature/gpb-update
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   rebar.config

no changes added to commit (use "git add" and/or "git commit -a")

@paulo-ferraz-oliveira
Copy link
Contributor Author

Try deleting _build (?) Let me try the same as you.

@wojtekmach
Copy link
Member

~/src/hex_core[feature/gpb-update]% rm -rf src/hex_pb_* ~/.cache/rebar3 _build

@paulo-ferraz-oliveira
Copy link
Contributor Author

~/src/hex_core[feature/gpb-update]% rm -rf src/hex_pb_* ~/.cache/rebar3 _build

Yeah, just saw it. 😄

@paulo-ferraz-oliveira
Copy link
Contributor Author

I get the same as you :) it's possible the plugin is locking a different version (but in its rebar.lock it's certainly not 4.17.6).

@wojtekmach
Copy link
Member

my rebar.lock is empty:

~/src/hex_core[feature/gpb-update]% cat rebar.lock
[].

@wojtekmach
Copy link
Member

oh, you are saying the plugin locks a certain version, never mind.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented May 20, 2021

What's odd is that gpb is showing up inside _build/dev/lib (4.17.0) and _build/dev/plugins (4.17.6). rebar3's picking one of them 🤔

We should be able to dictate what happens, at a higher level
@paulo-ferraz-oliveira
Copy link
Contributor Author

I pushed a small update, to tweak the plugin's dep. list. Delete _build and restart. I'm also asking around, in rebar3's Slack if there's a better solution.

@wojtekmach wojtekmach merged commit 85f303b into hexpm:master May 20, 2021
@wojtekmach
Copy link
Member

Fantastic, thank you!

@paulo-ferraz-oliveira
Copy link
Contributor Author

Cool.

@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/gpb-update branch May 26, 2021 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants