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

[sbt 1.0] Make publishTo & otherResolvers TaskKey's #2662

Merged
merged 1 commit into from Sep 29, 2016

Conversation

Projects
None yet
3 participants
@dwijnand
Member

dwijnand commented Jul 8, 2016

Fixes #2059

@dwijnand dwijnand added the in progress label Jul 8, 2016

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jul 8, 2016

Member

dependency-management/snapshot-resolution provides an interesting case. It currently has:

    publishMavenStyle := (publishTo.value match {
      case Some(repo) =>
        repo match {
          case repo: PatternsBasedRepository => repo.patterns.isMavenCompatible
          case _: RawRepository => false // TODO - look deeper
          case _: MavenRepository => true
          case _: MavenCache => true
          case _ => false  // TODO - Handle chain repository?
        }
      case _ => true
    })

That will no longer work as publishMavenStyle is a setting and publishTo is a task with this PR.

Should I fix it for the test, or shall I make publishMavenStyle a task? /cc @eed3si9n

Member

dwijnand commented Jul 8, 2016

dependency-management/snapshot-resolution provides an interesting case. It currently has:

    publishMavenStyle := (publishTo.value match {
      case Some(repo) =>
        repo match {
          case repo: PatternsBasedRepository => repo.patterns.isMavenCompatible
          case _: RawRepository => false // TODO - look deeper
          case _: MavenRepository => true
          case _: MavenCache => true
          case _ => false  // TODO - Handle chain repository?
        }
      case _ => true
    })

That will no longer work as publishMavenStyle is a setting and publishTo is a task with this PR.

Should I fix it for the test, or shall I make publishMavenStyle a task? /cc @eed3si9n

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jul 8, 2016

Member

For now I (somewhat) fixed it locally to that scripted test, just so Travis would go green. But let me know.

Member

dwijnand commented Jul 8, 2016

For now I (somewhat) fixed it locally to that scripted test, just so Travis would go green. But let me know.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Jul 15, 2016

Member

So we now know it's possible, why do we want this again?

Member

eed3si9n commented Jul 15, 2016

So we now know it's possible, why do we want this again?

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jul 15, 2016

Member

The initial feature request (#2059) said because they wanted to define publishTo in terms of credentials, which is a task.

I'm not sure I see a strong use-case there, but, without intending to sound like the devil's advocate, may I invert the question to you: why don't we want to do this?

For clarity, I don't feel strongly that we do, this was just easy pickings that was originally in the 1.0.0 milestone. So I'm happy not to, but we should close the feature request too then.

Member

dwijnand commented Jul 15, 2016

The initial feature request (#2059) said because they wanted to define publishTo in terms of credentials, which is a task.

I'm not sure I see a strong use-case there, but, without intending to sound like the devil's advocate, may I invert the question to you: why don't we want to do this?

For clarity, I don't feel strongly that we do, this was just easy pickings that was originally in the 1.0.0 milestone. So I'm happy not to, but we should close the feature request too then.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Jul 15, 2016

Member

publishTo seems to come up occasionally, so I was wondering what your take is. Recently I've seen an issue because bintray-sbt checks for Bintray credential file, and bintray-sbt was included into an Activator template. @2m I think also brought this up.

It's a classic setting vs task debate. From a common sense point of view where you publish your artifacts should not change over the lifetime of an sbt session. Turning it into a runtime behavior seems like an incidental complexity because it might want to depend on things that are tasks. I'm leaning towards merging this, but I want to see what's going on.

Member

eed3si9n commented Jul 15, 2016

publishTo seems to come up occasionally, so I was wondering what your take is. Recently I've seen an issue because bintray-sbt checks for Bintray credential file, and bintray-sbt was included into an Activator template. @2m I think also brought this up.

It's a classic setting vs task debate. From a common sense point of view where you publish your artifacts should not change over the lifetime of an sbt session. Turning it into a runtime behavior seems like an incidental complexity because it might want to depend on things that are tasks. I'm leaning towards merging this, but I want to see what's going on.

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jul 15, 2016

Member

Sounds like you mean sbt/sbt-bintray#88.

There's also @2m's needs-a-rework PR sbt/sbt-bintray#69.

The argument for setting is that we feel it should be sufficient, and settings are more performant (because they're memoized).

The argument for task is that it's more flexible.

So, if you really wonder my take, I mildly bias in this case to allow for the flexibility. (For instance this same argument applied to version (#2171) is much different, because version is so cornerstone to a project).

But I'm happy to hold this PR for a little while, but please let's not put in on the dead-but-no-closed PRs stack - a closed PR can always be re-opened months later.

Member

dwijnand commented Jul 15, 2016

Sounds like you mean sbt/sbt-bintray#88.

There's also @2m's needs-a-rework PR sbt/sbt-bintray#69.

The argument for setting is that we feel it should be sufficient, and settings are more performant (because they're memoized).

The argument for task is that it's more flexible.

So, if you really wonder my take, I mildly bias in this case to allow for the flexibility. (For instance this same argument applied to version (#2171) is much different, because version is so cornerstone to a project).

But I'm happy to hold this PR for a little while, but please let's not put in on the dead-but-no-closed PRs stack - a closed PR can always be re-opened months later.

@2m

This comment has been minimized.

Show comment
Hide comment
@2m

2m Jul 20, 2016

Member

That is correct, @dwijnand. This is what I needed when working on bintray-sbt - to be able to make publishTo (currenty a setting) depend on credentials (currently a task), publishTo needs to become a task.

Member

2m commented Jul 20, 2016

That is correct, @dwijnand. This is what I needed when working on bintray-sbt - to be able to make publishTo (currenty a setting) depend on credentials (currently a task), publishTo needs to become a task.

@dwijnand dwijnand closed this Sep 1, 2016

@dwijnand dwijnand deleted the dwijnand:publishTo-TaskKey branch Sep 1, 2016

@dwijnand dwijnand removed the in progress label Sep 1, 2016

@dwijnand dwijnand restored the dwijnand:publishTo-TaskKey branch Sep 11, 2016

@dwijnand dwijnand reopened this Sep 11, 2016

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Sep 11, 2016

Member

Artifactory (see #2088) requires the publishing URL to have a ;build.timestamp=... suffix for SNAPSHOTs, as a guide for what SNAPSHOTs artefacts belong to the same version.

By publishTo being a setting defined in terms of current time (eg. ... + ";build.timestamp=" + new java.util.Date().getTime), the build user must remember to reload the build in order to have a new suffix for the next snapshot release.

Instead if publishTo per a task this particular issue would work correctly out the box, because publishTo is then read by publishConfiguration, which uses the same URL for all the artefacts in the same publish.

At least in terms of the sbt 0.13 codebase, the only other place publishTo is used is to define otherResolvers, which then because part of the definition of the ivyConfiguration.

So I think we should merge this, if TravisCI passes.

Member

dwijnand commented Sep 11, 2016

Artifactory (see #2088) requires the publishing URL to have a ;build.timestamp=... suffix for SNAPSHOTs, as a guide for what SNAPSHOTs artefacts belong to the same version.

By publishTo being a setting defined in terms of current time (eg. ... + ";build.timestamp=" + new java.util.Date().getTime), the build user must remember to reload the build in order to have a new suffix for the next snapshot release.

Instead if publishTo per a task this particular issue would work correctly out the box, because publishTo is then read by publishConfiguration, which uses the same URL for all the artefacts in the same publish.

At least in terms of the sbt 0.13 codebase, the only other place publishTo is used is to define otherResolvers, which then because part of the definition of the ivyConfiguration.

So I think we should merge this, if TravisCI passes.

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Sep 11, 2016

Member

Oh actually, let me know, so I'll write some notes before we merge.

Member

dwijnand commented Sep 11, 2016

Oh actually, let me know, so I'll write some notes before we merge.

@eed3si9n eed3si9n changed the title from Make publishTo & otherResolvers TaskKey's to [sbt 1.0] Make publishTo & otherResolvers TaskKey's Sep 29, 2016

@eed3si9n eed3si9n merged commit 87ef528 into sbt:1.0.x Sep 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dwijnand dwijnand deleted the dwijnand:publishTo-TaskKey branch Sep 29, 2016

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Sep 29, 2016

Member

Thank you

Member

dwijnand commented Sep 29, 2016

Thank you

@laughedelic laughedelic referenced this pull request Oct 2, 2017

Merged

Updating to sbt 1.x #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment