Update hex core to v0.6.8 #2213

starbelly · 2020-01-26T20:59:48Z

This update requires lock file version bump as it introduces non-compatible checksums via hex_core.

Vendor in hex_core at v0.6.8
Remove checksum in favor of new outer_checksum key/val
Change warn_vsn_once/0 to warn_vsn_once/1 so we can give a proper
error message and take appropriate action based on the lock file version.
Update rebar.lock to 1.2.0 format.

src/rebar_app_utils.erl

starbelly · 2020-01-26T21:07:08Z

src/rebar_config.erl

@@ -92,8 +92,17 @@ consult_lock_file(File) ->
 %% at most once.
 %% The warning can also be cancelled by configuring the `warn_config_vsn'
 %% OTP env variable.
-spec warn_vsn_once() -> ok.
-warn_vsn_once() ->
+-spec warn_vsn_once(string()) -> ok.


Initial ugly version, some de-duplication should happen here. @ferd @tsloughter I think for this case we should print the message and exit gracefully. There is no successful continuation for this case, but maybe trying to exit gracefully isn't worth it.

The message also could be much better

Question: Why do we have a semver style lock version? I would think an single integer would suffice, but I can also see where having the ability to maintain multiple maintenance versions of rebar3 would serve well from such a version schema.

I just went with the most descriptive version possible when I started supporting them. I had no idea what would be needed in the future. For example, we could eventually decide that patch versions are not worth warnings because we'd be changing formatting or something of the kind, adding details that were optional but sometimes missing, and so on. Otherwise the Major/Minor version should let a theoretical Rebar4 know it found the lockfile of a Rebar3 and know it's not meant for it (or go read in some legacy mode).

In the case here we could have argued that the change should go for a new file format version (2.0.0) since we're moving from inner to outer hash without changing the key, and so the meaning is incompatible. However, if the {pkg_hash, ...} attribute currently used in this PR was replaced with {pkg_ext_hash, ...} we could probably keep the 1.2.0 version: a tool that analyzes all the hashfiles could do so without having to care about which it gets. If we wanted to play real nice we'd also keep the old pkg_hash value so that technically we would warn in older rebar3 versions, but wouldn't need to actually error out on the file. It would make people's diffs a bit larger but the transition neater. We could probably get rid of the pkg_hash key later down the line if ever needed after a few releases of pkg_ext_hash-supporting versions.

To be clear, pkg_hash would contain inner checksums and pkg_ext_hash would contain outer? That makes sense to me. Do we want to work this into rebar_pkg_resource so that people are not forced into unlocking and re-locking per upgrading to the latest version of rebar?

Yeah; pkg_hash same as before, pkg_ext_hash the external checksums. We currently rewrite all lockfiles even if they're unchanged so as long as you write the new format down you should be fine -- Tristan has a PR to correct that, but the version upgrade is precisely the problem we're hitting our noses against right now in there.

Internally you can keep checking only the external hash, but this gives us a non-breakable way to keep working with older rebar3 copies.

Ahhh I gotcha yeah. Will have this ready to go this evening.

To note: This will need a little bit more ❤️ than anticipated. We have to update more modules than expected. Specifically, we are having to extend the app info tuple to contain both the new hash and the old hash (outer and inner). Most of the work is done now but have a few minor details to finish up tomorrow.

starbelly · 2020-01-26T22:19:45Z

src/rebar_config.erl

    end.

+warning_for_vsn([Maj, _dot,  Min, _dot, _Patch]) when Maj =:= $1 andalso Min =< $1 ->
+    ?WARN("Rebar3 detected a lock file incompatible with this version of rebar3. "


It might be worth giving more explanation here. That is, to specifically mention a checksum incompatibility, but not sure if most users will care about that detail.

That warning is a sign that we're fully breaking backwards compat. See my prior comment about the attribute names so that we may avoid that.

src/rebar_app_utils.erl

ferd · 2020-01-27T17:01:31Z

src/rebar_config.erl

@@ -92,8 +92,17 @@ consult_lock_file(File) ->
 %% at most once.
 %% The warning can also be cancelled by configuring the `warn_config_vsn'
 %% OTP env variable.
-spec warn_vsn_once() -> ok.
-warn_vsn_once() ->
+-spec warn_vsn_once(string()) -> ok.


I just went with the most descriptive version possible when I started supporting them. I had no idea what would be needed in the future. For example, we could eventually decide that patch versions are not worth warnings because we'd be changing formatting or something of the kind, adding details that were optional but sometimes missing, and so on. Otherwise the Major/Minor version should let a theoretical Rebar4 know it found the lockfile of a Rebar3 and know it's not meant for it (or go read in some legacy mode).

In the case here we could have argued that the change should go for a new file format version (2.0.0) since we're moving from inner to outer hash without changing the key, and so the meaning is incompatible. However, if the {pkg_hash, ...} attribute currently used in this PR was replaced with {pkg_ext_hash, ...} we could probably keep the 1.2.0 version: a tool that analyzes all the hashfiles could do so without having to care about which it gets. If we wanted to play real nice we'd also keep the old pkg_hash value so that technically we would warn in older rebar3 versions, but wouldn't need to actually error out on the file. It would make people's diffs a bit larger but the transition neater. We could probably get rid of the pkg_hash key later down the line if ever needed after a few releases of pkg_ext_hash-supporting versions.

ferd · 2020-01-27T17:02:49Z

src/rebar_config.erl

    end.

+warning_for_vsn([Maj, _dot,  Min, _dot, _Patch]) when Maj =:= $1 andalso Min =< $1 ->
+    ?WARN("Rebar3 detected a lock file incompatible with this version of rebar3. "


That warning is a sign that we're fully breaking backwards compat. See my prior comment about the attribute names so that we may avoid that.

test/rebar_pkg_SUITE.erl

starbelly · 2020-01-27T17:28:50Z

@ferd Yeah, so this would be non-backwards compat in its current form in that, yes you would have to unlock all your deps and relock. But let me revise, because we can indeed support both I think with a healthy warning.

(cherry picked from commit 08eb611)

starbelly · 2020-01-30T06:11:40Z

@ferd Ok, got a bunch of tests to update, but the implementation is ready for review, will try to get tests done in the morning.

ferd · 2020-01-30T15:41:39Z

src/rebar_app_utils.erl

-    dep_to_app(Parent, DepsDir, Name, Vsn, {pkg, PkgName, Vsn, Hash}, IsLock, State);
+    dep_to_app(Parent, DepsDir, Name, Vsn, {pkg, PkgName, Vsn, undefined, undefined}, IsLock, State);
+parse_dep(Parent, {Name, {pkg, PkgName, Vsn, OldHash,  Hash}, Level}, DepsDir, IsLock, State) when is_integer(Level) ->
+    dep_to_app(Parent, DepsDir, Name, Vsn, {pkg, PkgName, Vsn, OldHash, Hash}, IsLock, State);


don't we have a potential case for OldHash =/= undefined with also Hash =:= undefined ?

I thought we did, but now I'm not so sure. Running rebar3 will acquire and write the new locks, along with the old. Given that and if you've upgraded to this version of rebar3, there should be no need to take check against the old hash. Make sense?

I was thinking of what happens if you load an old version of a lock on your first time with the new registry?

It works. A new lockfile will be generated with old hashes and new hashes. The updated lock file you see in the PR was generated via the original, and with the old package registry. I also tried with old lock file and the new registry. It all just works, seems to good to be true, but give it a go I'd say.

Sounds good then. I didn't think they'd be there at the same time but I guess by that point they have been resolved already.

As discussed on slack, we will go ahead and add in some logic to check for the inner checksum if the check against the outer checksum fails for good measure.

starbelly · 2020-01-31T05:16:42Z

src/rebar_app_utils.erl

@@ -287,16 +287,19 @@ expand_deps_sources(Dep, State) ->
    rebar_app_info:t() when
      Source :: rebar_resource_v2:source().
 update_source(AppInfo, {pkg, PkgName, PkgVsn, Hash}, State) ->
-    case rebar_packages:resolve_version(PkgName, PkgVsn, Hash,
+    update_source(AppInfo, {pkg, PkgName, PkgVsn, undefined, Hash}, State);


This was needed to handle the case where we're running without locks, there may be a better place to tackle this but I didn't see one.

starbelly · 2020-02-02T13:58:05Z

@ferd This should be ready for a final review, nits welcome.

ferd

Minor changes to do but otherwise ready to go!

ferd · 2020-02-04T17:28:25Z

src/rebar_config.erl

@@ -156,7 +157,8 @@ maybe_comma([_|_]) -> io_lib:format(",~n", []).
 read_attrs(_Vsn, Locks, Attrs) ->
    %% Beta copy does not know how to expand attributes, but
    %% is ready to support it.
-    expand_locks(Locks, extract_pkg_hashes(Attrs)).
+    {OldHashes, NewHashes} =  extract_pkg_hashes(Attrs),
+    expand_locks(Locks, OldHashes, NewHashes).

 %% @private extract the package hashes from lockfile attributes, if any.
 -spec extract_pkg_hashes(list()) -> [binary()].


this typespec needs updating

ferd · 2020-02-04T17:30:48Z

src/rebar_config.erl

-    {NewLocks, Hashes} = split_locks(Locks, [], []),
-    case Hashes of
+    {NewLocks, OldHashes, NewHashes} = split_locks(Locks, [], [], []),
+    case OldHashes ++ NewHashes of


probably cheaper to check on:

case {OldHashes, NewHashes} of {[], []} -> {NewLocks, []}; _ -> ... end

since it won't need to append lists together only to never use the result.

Whole heartily agree and one of my favorite patterns 👍

ferd · 2020-02-04T17:33:49Z

src/rebar_pkg_resource.erl

+                    ?DEBUG("Expected hash ~64.16.0B does not match outer checksum of fetched package ~64.16.0B, but
+                           matches inner checksum ~64.16.0B",
+                        [RegistryChecksum, Checksum, OldChecksum]),
+                    ok;


I think we should, at least for the RC-1, crash here regardless, but keep the new logging message. This case could technically be the attack the outer-checksum is planning to protect against.

sounds good

- Vendor in hex_core at v0.6.8 - Rename checksum to inner_checksum and deprecate in favor of new outer_checksum key/val - Change warn_vsn_once/0 to warn_vsn_once/1 so we can give a proper error message and take appropriate action. - Update rebar.lock to 1.2.0 format with continued support third-party that still may make use of inner checksum via continued persistence of inner checksums. - Changed app info tuple (package representation) to support inner and outer hashes for backwards compat support (see above)

(cherry picked from commit 08eb611)

starbelly mentioned this pull request Jan 26, 2020

Introduce rebar_uri to avoid deprecation warnings on OTP 23 [master] #2195

Merged

starbelly commented Jan 26, 2020

View reviewed changes

src/rebar_app_utils.erl Show resolved Hide resolved

starbelly commented Jan 26, 2020

View reviewed changes

starbelly force-pushed the update-hex-core-to-v0.6.5 branch from 24791b2 to cd60138 Compare January 26, 2020 22:10

starbelly marked this pull request as ready for review January 26, 2020 22:11

starbelly commented Jan 26, 2020

View reviewed changes

ferd reviewed Jan 27, 2020

View reviewed changes

michaelklishin added a commit to michaelklishin/rebar3 that referenced this pull request Jan 28, 2020

Undo r3_hex_api changes in favor of erlang#2213

08eb611

michaelklishin added a commit to michaelklishin/rebar3 that referenced this pull request Jan 28, 2020

Undo r3_hex_api changes in favor of erlang#2213

2f4521e

(cherry picked from commit 08eb611)

starbelly force-pushed the update-hex-core-to-v0.6.5 branch from c734db9 to 379db90 Compare January 30, 2020 06:10

ferd reviewed Jan 30, 2020

View reviewed changes

starbelly commented Jan 31, 2020

View reviewed changes

starbelly force-pushed the update-hex-core-to-v0.6.5 branch from 95bf4a2 to 970b3d6 Compare February 2, 2020 13:47

starbelly closed this Feb 2, 2020

starbelly reopened this Feb 2, 2020

starbelly force-pushed the update-hex-core-to-v0.6.5 branch 3 times, most recently from 3504411 to f9b5566 Compare February 2, 2020 14:03

starbelly changed the title ~~Update hex core to v0.6.5~~ Update hex core to v0.6.6 Feb 3, 2020

starbelly mentioned this pull request Feb 3, 2020

Add vendor_hex_core.sh #2219

Merged

starbelly changed the title ~~Update hex core to v0.6.6~~ Update hex core to v0.6.7 Feb 3, 2020

starbelly changed the title ~~Update hex core to v0.6.7~~ Update hex core to v0.6.8 Feb 4, 2020

ferd approved these changes Feb 4, 2020

View reviewed changes

starbelly force-pushed the update-hex-core-to-v0.6.5 branch from 8a590bc to 46765c8 Compare February 5, 2020 02:47

ferd merged commit 310a4ad into erlang:master Feb 5, 2020

ferd pushed a commit to ferd/rebar3 that referenced this pull request Feb 27, 2020

Undo r3_hex_api changes in favor of erlang#2213

104e908

(cherry picked from commit 08eb611)

seriyps mentioned this pull request Apr 10, 2020

rebar3 hex publish incompatible with pkg_hash_ext rebar.lock erlef/rebar3_hex#154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update hex core to v0.6.8 #2213

Update hex core to v0.6.8 #2213

starbelly commented Jan 26, 2020 •

edited

Loading

starbelly Jan 26, 2020

starbelly Jan 26, 2020

starbelly Jan 26, 2020

ferd Jan 27, 2020

starbelly Jan 28, 2020

ferd Jan 28, 2020

starbelly Jan 28, 2020

starbelly Jan 29, 2020

starbelly Jan 26, 2020

ferd Jan 27, 2020

ferd Jan 27, 2020

ferd Jan 27, 2020

starbelly commented Jan 27, 2020

starbelly commented Jan 30, 2020

ferd Jan 30, 2020

starbelly Jan 30, 2020

ferd Jan 30, 2020

starbelly Jan 30, 2020 •

edited

Loading

ferd Jan 30, 2020

starbelly Feb 2, 2020

starbelly Jan 31, 2020

starbelly commented Feb 2, 2020

ferd left a comment

ferd Feb 4, 2020

ferd Feb 4, 2020

starbelly Feb 4, 2020

ferd Feb 4, 2020

starbelly Feb 4, 2020

Update hex core to v0.6.8 #2213

Update hex core to v0.6.8 #2213

Conversation

starbelly commented Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starbelly commented Jan 27, 2020

starbelly commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starbelly Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starbelly commented Feb 2, 2020

ferd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starbelly commented Jan 26, 2020 •

edited

Loading

starbelly Jan 30, 2020 •

edited

Loading