erlef / rebar3_hex

Rebar3 Hex library
Apache License 2.0
101 stars 49 forks source link

validate error: deprecated field maintainers found #125

Closed benoitc closed 4 years ago

benoitc commented 5 years ago

Why this field have been deprecated? It is used in some workflow? Is there any related ticket that could explain the change?

Also why publishing is failing when present? Since there is no replacement, it shouldn't fail, just notice IMO.

wojtekmach commented 5 years ago

maintainers field has been deprecated (simply ignored when publishing) on the server for about a year, here is the rationale: https://github.com/hexpm/hex/blob/master/CHANGELOG.md#ignoring-maintainers-field.

Wiadomość napisana przez Benoit Chesneau notifications@github.com w dniu 03.07.2019, o godz. 06:59:

Why this field have been deprecated? It is used in some workflow? Is there any related ticket that could explain the change?

Also why publishing is failing when present? Since there is no replacement, it shouldn't fail, just notice IMO.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

benoitc commented 5 years ago

OK. But whatever the reason i don't see why it should return an error. Metadata in app.src czn be used for other purposes than publishing on hex. I think it should be simply ignored for the reason above.

On Wed 3 Jul 2019 at 07:23, Wojtek Mach notifications@github.com wrote:

maintainers field has been deprecated (simply ignored when publishing) on the server for about a year, here is the rationale: https://github.com/hexpm/hex/blob/master/CHANGELOG.md#ignoring-maintainers-field .

Wiadomość napisana przez Benoit Chesneau notifications@github.com w dniu 03.07.2019, o godz. 06:59:

Why this field have been deprecated? It is used in some workflow? Is there any related ticket that could explain the change?

Also why publishing is failing when present? Since there is no replacement, it shouldn't fail, just notice IMO.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tsloughter/rebar3_hex/issues/125?email_source=notifications&email_token=AAADRIXO7YUKEV2UR66L3SLP5QZWZA5CNFSM4H5B4ZHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZDJPWI#issuecomment-507942873, or mute the thread https://github.com/notifications/unsubscribe-auth/AAADRIVZCUDVQY5S4CEODYLP5QZWZANCNFSM4H5B4ZHA .

-- Sent from my Mobile

ferd commented 5 years ago

https://github.com/tsloughter/rebar3_hex/pull/85 introduced the behaviour. My guess from looking at the code is that this was a minimal fix that fit the existing validation structures, but didn't necessarily look further than that.

That change came around 3.7.0, where we had major crashing problems because of the new rebar3 providers for git resources and hex resources using hex_core rather than custom code. When backwards incompatible changes happened in hex_core and that rebar3_hex started using them, the plugin would load the lib in memory, clashing with the version rebar3 needed for itself (since they had to share the same namespace and could prove to be trouble when the plugin hijacked the lib version needed by rebar3). Every time rebar3 would try to fetch a package (including a patched plugin!), it would violently die on undefined function calls.

Given limited time, @starbelly and I had a chat where it felt like a better idea for him to spend his limited contribution time on helping with the hex_core vendoring efforts within rebar3 (giving a new namespace to prevent clashes) and paths forwards for this plugin. Considering the scale of the problems then and that the hard deprecation was already public, we simply ran out of time / it was too low priority to be modified and made to be nicer than it is right now, but we at least now have safe upgrade path for hex libs that won't kill rebar3 wholesale.

tsloughter commented 5 years ago

I like it being an error because it is easy to remove by the user, so why not make sure it gets removed instead of letting it sit there getting out of sync with hex.

benoitc commented 5 years ago

@tsloughter but some people are using such metadata for their own source management system, and it can be the same for the future metadata that can be removed.

I propose to patch the current implementation by whitelisting the metadata handled by hex. I was thinking some namespacing either with a prefix or a map in the env could be done also but that will be less user friendly and annoying for old users.

tsloughter commented 5 years ago

@benoitc good point. Since putting any metadata in .app.src is considered "wrong" by OTP I didn't think anyone else would be using it.

I suppose it is fine making it a warning and in the end need to rethink how we do this metadata. Maybe just putting it under a hexpm key in the .app.src. Though since it is considered "wrong" to be putting this stuff in there we should get it officially added to the .app spec.

wojtekmach commented 5 years ago

Since putting any metadata in .app.src is considered "wrong" by OTP I didn't think anyone else would be using it.

Does OTP have a spec for .app and .app.src? I only found it for the former [1].

I personally like the idea of namespacing but I also understand friction in moving existing packages over to use them. Can we solve friction with crowdsourcing or tooling (ala go fmt)?

In mix/hex we allow to do the following:

  def project() do
    [
      app: :foo,
      ...,
      package: ...,
      hex: ...,
    ]
  end

Where under package we have metadata like licenses, links etc (this stuff ends up in metadata.config in package tarball) and under hex we allow configuring the client so things like api_key, api_url etc (this does not). Neither package nor hex namespace contents land in the final .app file built by Mix.

Under package we notably allow setting optional organization key for Hex.pm private packages (we don't store it in metadata.config in tarball either). We had reports that folks typed this as organisation, which we silently ignored, only to found out the package was published publicly. Nowadays, if someone types organisation there, we raise error with an explanation.

I mention this because as you can see silently ignoring fields may have consequences. I think warnings are ok, but I feel like it's so easy to miss them and for that reason I personally prefer errors. (At the moment we neither warn nor error on unrecognised fields in mix/hex besides the case mentioned above.) In hindsight, feels like having a longer deprecation period with a warning would have been better for the maintainers field in Rebar.

[1] http://erlang.org/doc/man/app.html

tsloughter commented 5 years ago

I never got around to it but the discussion around the app file was to add a custom metadata key to add this stuff under https://github.com/erlang/otp/pull/1793

starbelly commented 5 years ago

The only thing we can do at this point is switch to warnings vs hard errors. I'm ok with doing that PR if @tsloughter agrees.

tsloughter commented 5 years ago

OK, might as well.

starbelly commented 5 years ago

Ok, I will get a PR together later tonight or tomorrow. Have to re-validate what is fatal and what is not within hexpm currently.

starbelly commented 4 years ago

This was resolved via https://github.com/tsloughter/rebar3_hex/pull/135 and thus this issue can be closed.

starbelly commented 4 years ago

Going to close this issue per #135