Throw error if publishing a non-pre package that has pre-dependencies #333

fteem · 2016-12-25T21:49:50Z

Took a quick shot at #320 and I'd like some feedback because I think it needs more polishing. Also, should it throw an error or just show a warning? Fire away! 😃

milmazz

I added a few comments, hope this helps.

milmazz · 2016-12-26T00:21:34Z

lib/mix/tasks/hex/publish.ex

+    if !is_pre_version(meta) && has_pre_requirements(meta) do
+      # Better wording would be nice...
+      Mix.raise "Cannot publsh non-pre package that depends on a pre package"
+    end


The exception message should begin with lowercase, wdyt about this: "A stable package release cannot have a pre-release dependency"

milmazz · 2016-12-26T00:23:39Z

test/mix/tasks/hex/publish_test.exs

@@ -192,4 +192,23 @@ defmodule Mix.Tasks.Hex.PublishTest do
  after
    purge [ReleaseMeta.Mixfile]
  end
+
+  test "create a non-pre package with pre-package deps" do


What do you think about: create a stable package release with a pre-release dependency?

milmazz · 2016-12-26T00:30:15Z

lib/mix/tasks/hex/publish.ex

+  defp has_pre_requirements(meta) do
+    meta.requirements
+    |> Enum.map(fn(req) -> req[:requirement] end)
+    |> Enum.any?(fn(req) -> req =~ "pre" end)


What about other common suffixes as: -rc, -alpha, -beta?. Also, you should consider the following:

A pre-release version number MAY be denoted by appending an arbitrary string immediately following the patch version and a dash. The string MUST be comprised of only alphanumerics plus dash [0-9A-Za-z-]. Pre-release versions satisfy but have a lower precedence than the associated normal version. Precedence SHOULD be determined by lexicographic ASCII sort order. For instance: 1.0.0-alpha1 < 1.0.0-beta1 < 1.0.0-beta2 < 1.0.0-rc1 < 1.0.0.

Source: http://semver.org/spec/v1.0.0.html#semantic-versioning-specification-semver

ericmj · 2016-12-26T14:21:57Z

lib/mix/tasks/hex/publish.ex

+  end
+
+  defp is_pre_version(meta) do
+    meta.version =~ "pre"


This and the same check above should be done like this:

version = Hex.Version.parse!(string) version.pre != []

wojtekmach

Looks pretty good! We still need to correctly handle more complicated version requirements

wojtekmach · 2016-12-27T13:29:17Z

lib/mix/tasks/hex/publish.ex

+    meta.requirements
+    |> Enum.map(fn(req) -> req[:requirement] end)
+    |> Enum.any?(fn(req) -> is_pre_version(req) end)
+  end


@fteem @ericmj wdyt about the following refactoring?

defp has_pre_requirements?(meta) do meta.requirements |> Enum.map(& &1.requirement) |> Enum.any?(pre_requirement?/1) end

wojtekmach · 2016-12-27T13:30:58Z

lib/mix/tasks/hex/publish.ex

@@ -190,6 +194,17 @@ defmodule Mix.Tasks.Hex.Publish do
    Hex.Shell.yes?("Proceed?")
  end

+  defp is_pre_version(version) do
+    version = Hex.Version.parse!(version)


version should actually be version_requirement for which there's a Hex.Version.parse_requirement!. There's unfortunately no .pre field on Version.Requirement struct, we'd probably need to extract this information ourselves.

PS Having a function to determine if a version requirement allows prereleases could be a nice addition to Elixir. cc @ericmj

actually, a quick'n'dirty way of determining this is simply doing:

String.contains?(requirement, "-")

Haha, after @milmazz left his comment about matching other version suffixes I though exactly about this approach, but I'd prefer to use Hex's correct way over a hack :)

We check the pre-release for each component of the requirement so: ~> 1.0-dev and ~> 2.0 does not match 2.1.0-dev. But there is no way of knowing what versions we expect so I think the String.contains? check is our best bet.

wojtekmach · 2016-12-27T13:37:08Z

test/support/release_samples.ex

+defmodule ReleasePreDeps.Mixfile do
+  def project do
+    [app: :release_b, description: "bar", version: "0.0.1",
+     deps: [{:ex_doc, "0.0.1-pre"}],


please change to ~> 0.0.1-pre

wojtekmach · 2016-12-27T13:59:09Z

Oh, btw: IMHO we should warn (instead of throwing an error) to give more flexibility to folks but I don't have a strong opinion about it.

ericmj · 2016-12-27T20:04:11Z

Oh, btw: IMHO we should warn (instead of throwing an error) to give more flexibility to folks but I don't have a strong opinion about it.

I think we should error but allow an override flag. In my experience people don't read the warnings on mix hex.publish :(.

fteem · 2016-12-27T22:33:58Z

@ericmj I added a --force flag, while the task itself throws an error. I took a dive in Hex.Version and tried to expose a pre field in the struct but I couldn't get it to work, so I used @wojtekmach's hack to detect the version requirements. Let me know your thoughts.

wojtekmach

@fteem I've spoken with @ericmj about this issue and we've decided that it would be better to display an error and not allow --force flag in this situation. I think it would be a reasonable assumption that a stable packages can depend only on other stable ones. If someone needs to depend on a pre-release version then that should be a pre-release as well. We could revisit this decision if someone comes up with a use case.

Sorry for going back'n'forth; do you mind updating the PR accordingly?

michalmuskala · 2017-01-27T16:59:00Z

A use case for depending on pre-releases that happened in the past with ecto, is that you accept versions of the dependency since that beta release, but later stable releases are fine too. SemVer is all about expressing minimal requirements, so it might make sense in that context.

Example: https://hex.pm/packages/ecto/2.0.6 depends on sbroker ~> 1.0-beta

Another case is preparing for a release of a new version of "parent" package by allowing prerelease, e.g ~> 2.0 or ~> 2.1-beta. That one was happening with packages depending on ecto when we were preparing for a stable 2.1 release.

To sum up: I don't agree with such a warning/error.

josevalim · 2017-01-27T20:38:10Z

@michalmuskala the question is: if your software depends on beta software, can your software be production ready then? I would likely to at least put a blocker on front on this.

In regards to --force, I believe we have discussed it elsewhere but it sets a dangerous precedent. I believe we should have a flag per warning we may emit, otherwise it is easy to use --force for everything and bypass important checks. So this one would be --allow-pre-deps and so on.

ericmj · 2017-01-27T20:41:49Z

If we add an override it would need to be an override flag specific to this case, for example --allow-pre-deps, like @josevalim says. For now we would like to get a feel for when people need to use pre-deps without their project being a pre-release. Unfortunately the only way is to remove the option completely, if people have valid use-cases we can add the flag.

Another case is preparing for a release of a new version of "parent" package by allowing prerelease, e.g ~> 2.0 or ~> 2.1-beta. That one was happening with packages depending on ecto when we were preparing for a stable 2.1 release.

If users need the pre-release they have the option to add overrides for this.

ericmj

We should move this check to build.ex where we have the other package validation: https://github.com/hexpm/hex/blob/master/lib/mix/hex/build.ex#L68

ericmj · 2017-02-15T10:24:50Z

lib/mix/tasks/hex/publish.ex

@@ -190,6 +190,24 @@ defmodule Mix.Tasks.Hex.Publish do
    Hex.Shell.yes?("Proceed?")
  end

+  defp stable_release?(build, opts) do
+    if !pre_requirement?(build.meta.version) && has_pre_requirements?(build.meta) do


Since we are working with booleans we can change this to not pre_requirement?(build.meta.version) and has_pre_requirements?(build.meta).

ericmj · 2017-02-15T10:25:47Z

lib/mix/tasks/hex/publish.ex

+      false
+    end
+    true
+  end


Since this function raises and we don't care about the return value we can remove the booleans and change the name to check_pre_releases.

This function will raise an error if the package is stable but it has dependencies on pre-release packages.

ericmj

This looks good to me. Travis errors seem unrelated.

wojtekmach · 2017-04-18T14:03:40Z

Thanks @fteem!

milmazz reviewed Dec 26, 2016

View reviewed changes

ericmj reviewed Dec 26, 2016

View reviewed changes

fteem force-pushed the pre-deps-package branch from 6ad1c88 to 0ed6c37 Compare December 26, 2016 14:34

wojtekmach requested changes Dec 27, 2016

View reviewed changes

fteem force-pushed the pre-deps-package branch 2 times, most recently from 3960bf6 to 4d821bf Compare December 27, 2016 22:27

wojtekmach approved these changes Dec 27, 2016

View reviewed changes

wojtekmach requested changes Jan 27, 2017

View reviewed changes

wojtekmach approved these changes Feb 14, 2017

View reviewed changes

ericmj requested changes Feb 15, 2017

View reviewed changes

fteem added 2 commits March 20, 2017 20:47

Add Mix.Tasks.Hex.Build.pre_requirement?/1

3781e87

Add Mix.Tasks.Hex.Build.has_pre_prequirements?/1

969edf2

fteem force-pushed the pre-deps-package branch from 660c8c2 to 3c8d879 Compare March 20, 2017 19:48

elixir-bot approved these changes Mar 20, 2017

View reviewed changes

Add Mix.Tasks.Hex.Build.raise_if_unstable_dependencies!/1

eb99c85

This function will raise an error if the package is stable but it has dependencies on pre-release packages.

fteem force-pushed the pre-deps-package branch from 3c8d879 to eb99c85 Compare March 20, 2017 20:15

elixir-bot approved these changes Mar 20, 2017

View reviewed changes

ericmj approved these changes Apr 18, 2017

View reviewed changes

wojtekmach merged commit 59e9942 into hexpm:master Apr 18, 2017

wojtekmach mentioned this pull request Apr 18, 2017

Warn or error if publishing a non-pre package that has pre-dependencies #320

Closed

fteem deleted the pre-deps-package branch June 6, 2017 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw error if publishing a non-pre package that has pre-dependencies #333

Throw error if publishing a non-pre package that has pre-dependencies #333

fteem commented Dec 25, 2016 •

edited

milmazz left a comment

milmazz Dec 26, 2016

milmazz Dec 26, 2016

milmazz Dec 26, 2016

ericmj Dec 26, 2016

wojtekmach left a comment

wojtekmach Dec 27, 2016

wojtekmach Dec 27, 2016

wojtekmach Dec 27, 2016 •

edited

fteem Dec 27, 2016

ericmj Dec 27, 2016 •

edited

wojtekmach Dec 27, 2016

wojtekmach commented Dec 27, 2016

ericmj commented Dec 27, 2016

fteem commented Dec 27, 2016

wojtekmach left a comment

michalmuskala commented Jan 27, 2017 •

edited

josevalim commented Jan 27, 2017

ericmj commented Jan 27, 2017

ericmj left a comment

ericmj Feb 15, 2017

ericmj Feb 15, 2017

ericmj left a comment

wojtekmach commented Apr 18, 2017

Throw error if publishing a non-pre package that has pre-dependencies #333

Throw error if publishing a non-pre package that has pre-dependencies #333

Conversation

fteem commented Dec 25, 2016 • edited

milmazz 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

wojtekmach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtekmach Dec 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmj Dec 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtekmach commented Dec 27, 2016

ericmj commented Dec 27, 2016

fteem commented Dec 27, 2016

wojtekmach left a comment

Choose a reason for hiding this comment

michalmuskala commented Jan 27, 2017 • edited

josevalim commented Jan 27, 2017

ericmj commented Jan 27, 2017

ericmj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmj left a comment

Choose a reason for hiding this comment

wojtekmach commented Apr 18, 2017

fteem commented Dec 25, 2016 •

edited

wojtekmach Dec 27, 2016 •

edited

ericmj Dec 27, 2016 •

edited

michalmuskala commented Jan 27, 2017 •

edited