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

Macro implementation when using triggers is not working #1444

Closed
huntc opened this Issue Jul 10, 2014 · 17 comments

Comments

Projects
None yet
@huntc

huntc commented Jul 10, 2014

This works:

discoveredDist <<= (dist in ReactiveRuntime)
  .storeAs(discoveredDist in Global)
  .triggeredBy(dist in ReactiveRuntime)

however this doesn't:

discoveredDist := (dist in ReactiveRuntime)
  .storeAs(discoveredDist in Global)
  .triggeredBy(dist in ReactiveRuntime)
  .value

@jsuereth jsuereth added the Bug label Aug 8, 2014

@eed3si9n eed3si9n added this to the 0.13.7 milestone Aug 8, 2014

@eed3si9n eed3si9n modified the milestone: 0.13.7 Nov 4, 2014

@eed3si9n eed3si9n added this to the 0.13.14 milestone Nov 3, 2016

@dwijnand dwijnand added the Regression label Nov 3, 2016

@liff

This comment has been minimized.

Show comment
Hide comment
@liff

liff Nov 3, 2016

Minimal repro:

// build.sbt
val hello = taskKey[Unit]("hello")
val world = taskKey[Unit]("world")

hello := println("hello")
world := println("world")

world := (world triggeredBy hello).value // prints "hello", should print "hello\nworld"

// world <<= world triggeredBy hello // works

Run with sbt hello.

liff commented Nov 3, 2016

Minimal repro:

// build.sbt
val hello = taskKey[Unit]("hello")
val world = taskKey[Unit]("world")

hello := println("hello")
world := println("world")

world := (world triggeredBy hello).value // prints "hello", should print "hello\nworld"

// world <<= world triggeredBy hello // works

Run with sbt hello.

@cunei

This comment has been minimized.

Show comment
Hide comment
@cunei

cunei Nov 23, 2016

Contributor

It seems this issue is only incidentally related to the macro expansion. The code that is generated when this expression is expanded:

world := (world triggeredBy hello).value

in the end happens to be similar to:

world <<= ((world triggeredBy hello) map ( a => a ))

In the latter expression, the triggeredBy for some reason gets ignored; that happens on 0.12.x as well. I'm doing some debugging now.

Contributor

cunei commented Nov 23, 2016

It seems this issue is only incidentally related to the macro expansion. The code that is generated when this expression is expanded:

world := (world triggeredBy hello).value

in the end happens to be similar to:

world <<= ((world triggeredBy hello) map ( a => a ))

In the latter expression, the triggeredBy for some reason gets ignored; that happens on 0.12.x as well. I'm doing some debugging now.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Dec 21, 2016

Member

old logic

If we consider a task to consists of two parts, value function and an attribute list, the existing behavior is defensible. <<= binds both the parts to the LHS task key whereas := creates a brand new task using the given value and a blank attribute list.

In that logic,

world := (world triggeredBy hello).value

is not much different from

world := {
  val x = (world triggeredBy hello).value
  123
}

or

world := 123

Functor law

The part that starts to break down is map.
The first functor law states that mapping with identity should not modify the original functor.
See also http://eed3si9n.com/herding-cats/Functor.html
It might not be strictly enforced, but the law matches the intuitive feel people on the standard collections too.

new ideas?

Even though it's forgetful, focusing only on the value using := is very valuable in flattening the learning curve of sbt.

One workaround might be to track triggers as a separate key:

onComplete in hello += world

This would move the trigger as if they are values, and at the end of the loading we can put them to the appropriate task attributes.

Member

eed3si9n commented Dec 21, 2016

old logic

If we consider a task to consists of two parts, value function and an attribute list, the existing behavior is defensible. <<= binds both the parts to the LHS task key whereas := creates a brand new task using the given value and a blank attribute list.

In that logic,

world := (world triggeredBy hello).value

is not much different from

world := {
  val x = (world triggeredBy hello).value
  123
}

or

world := 123

Functor law

The part that starts to break down is map.
The first functor law states that mapping with identity should not modify the original functor.
See also http://eed3si9n.com/herding-cats/Functor.html
It might not be strictly enforced, but the law matches the intuitive feel people on the standard collections too.

new ideas?

Even though it's forgetful, focusing only on the value using := is very valuable in flattening the learning curve of sbt.

One workaround might be to track triggers as a separate key:

onComplete in hello += world

This would move the trigger as if they are values, and at the end of the loading we can put them to the appropriate task attributes.

@gabro

This comment has been minimized.

Show comment
Hide comment
@gabro

gabro Jan 3, 2017

Is there a workaround to this issue that will achieve the same behavior without triggering a deprecation warning in sbt?

gabro commented Jan 3, 2017

Is there a workaround to this issue that will achieve the same behavior without triggering a deprecation warning in sbt?

@dwijnand dwijnand self-assigned this Jan 10, 2017

dwijnand added a commit to dwijnand/sbt that referenced this issue Jan 11, 2017

@cunei

This comment has been minimized.

Show comment
Hide comment
@cunei

cunei Jan 11, 2017

Contributor

@dwijnand 👍

Contributor

cunei commented Jan 11, 2017

@dwijnand 👍

dwijnand added a commit to dwijnand/sbt that referenced this issue Jan 12, 2017

dwijnand added a commit to dwijnand/sbt that referenced this issue Jan 13, 2017

eed3si9n added a commit that referenced this issue Jan 13, 2017

Merge pull request #2908 from dwijnand/fix-triggeredBy
Fix triggeredBy/storeAs/etc using :=. Fixes #1444

@dwijnand dwijnand closed this Jan 13, 2017

@dwijnand dwijnand removed the in progress label Jan 13, 2017

xuwei-k referenced this issue in skinny-framework/sbt-servlet-plugin Jan 14, 2017

@jcranky

This comment has been minimized.

Show comment
Hide comment
@jcranky

jcranky Mar 20, 2017

This fix is not available on 0.13.x yet, right?

jcranky commented Mar 20, 2017

This fix is not available on 0.13.x yet, right?

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Mar 20, 2017

Member

It's available in 0.13.14-RC1.

Member

dwijnand commented Mar 20, 2017

It's available in 0.13.14-RC1.

@jcranky

This comment has been minimized.

Show comment
Hide comment
@jcranky

jcranky Mar 20, 2017

thanks, worked fine with that version :)

jcranky commented Mar 20, 2017

thanks, worked fine with that version :)

dwijnand added a commit to dwijnand/sbt that referenced this issue Apr 3, 2017

@fdietze

This comment has been minimized.

Show comment
Hide comment
@fdietze

fdietze Jun 4, 2017

Hi, this does not seem to be fixed for me:

refreshBrowsers <<= refreshBrowsers.triggeredBy(WebKeys.assets in Assets)

works, while

refreshBrowsers := refreshBrowsers.triggeredBy(WebKeys.assets in Assets)

does not.

sbt.version=0.13.15

Project source: https://github.com/woost/wust2/blob/dff8fdcdd13d39dfaf51e61b9ec251b99d1e619c/build.sbt#L232

Do you need more information?

fdietze commented Jun 4, 2017

Hi, this does not seem to be fixed for me:

refreshBrowsers <<= refreshBrowsers.triggeredBy(WebKeys.assets in Assets)

works, while

refreshBrowsers := refreshBrowsers.triggeredBy(WebKeys.assets in Assets)

does not.

sbt.version=0.13.15

Project source: https://github.com/woost/wust2/blob/dff8fdcdd13d39dfaf51e61b9ec251b99d1e619c/build.sbt#L232

Do you need more information?

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Jun 4, 2017

Member

Did you miss '.value' intentionally in the code snippet that uses :=?

Member

jvican commented Jun 4, 2017

Did you miss '.value' intentionally in the code snippet that uses :=?

@fdietze

This comment has been minimized.

Show comment
Hide comment
@fdietze

fdietze Jun 4, 2017

Good catch! That was not intentional, it works with .value. Thank you!

It's strange though that I didn't get a compile error...

fdietze commented Jun 4, 2017

Good catch! That was not intentional, it works with .value. Thank you!

It's strange though that I didn't get a compile error...

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Jun 6, 2017

Member

It's because the expected Unit discards value, so it didn't catch the lack of .value invocation.

Member

eed3si9n commented Jun 6, 2017

It's because the expected Unit discards value, so it didn't catch the lack of .value invocation.

@mborra

This comment has been minimized.

Show comment
Hide comment
@mborra

mborra Jun 9, 2017

Hi to all, I have tried with :

compile in MultiJvm := ( compile in MultiJvm ).triggeredBy( compile in Test ).value

instead of:

compile in MultiJvm <<= ( compile in MultiJvm ) triggeredBy ( compile in Test )

But doesn't works

mborra commented Jun 9, 2017

Hi to all, I have tried with :

compile in MultiJvm := ( compile in MultiJvm ).triggeredBy( compile in Test ).value

instead of:

compile in MultiJvm <<= ( compile in MultiJvm ) triggeredBy ( compile in Test )

But doesn't works

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Jun 9, 2017

Member

Which sbt version?

Member

jvican commented Jun 9, 2017

Which sbt version?

@mborra

This comment has been minimized.

Show comment
Hide comment
@mborra

mborra Jun 9, 2017

Sorry, 0.13.13

mborra commented Jun 9, 2017

Sorry, 0.13.13

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Jun 9, 2017

Member

This thread says that the fix for triggeredBy is only available in 0.13.14. You need to update.

Member

jvican commented Jun 9, 2017

This thread says that the fix for triggeredBy is only available in 0.13.14. You need to update.

@mborra

This comment has been minimized.

Show comment
Hide comment
@mborra

mborra Jun 9, 2017

Sorry, you're right.

mborra commented Jun 9, 2017

Sorry, you're right.

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