-
Notifications
You must be signed in to change notification settings - Fork 90
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
Introduce TaskValue to avoid out-of-graph execution #114
Conversation
val projInTaskGraph2 = project dependsOn projInTaskGraph1 settings ( | ||
BuildInfoPlugin.buildInfoDefaultSettings, | ||
addBuildInfoToConfig(Test), | ||
buildInfoKeys in Test += (fullClasspath in Compile).taskValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In sbt/sbt#2943 I'm injecting .taskValue
as macro when the LHS is Initialize[Seq[Task[...]]]
.
Would you be able to take advantage of that if buildInfoKeys
were SettingKey[Seq[Task[String]]
, and you unify other miscellaneous build info entries to a Task[...]
instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure - maybe. But how are you going to change the type of buildInfoKeys
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just change the type and bump up minor version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about any plugin that depends on sbt-buildinfo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am personally unaware of any such plugin, and I don't feel obligation to keep bincompat as long as we bump to 0.8.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is going to work because then this no longer works:
buildInfoKeys := Seq(name, version, scalaVersion, sbtVersion)
which is what the README details.
we're going to have to look into the macro-to-expand-to-taskValue way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or (and this was the same argument as sbt/sbt#2943) own the fact that TaskKey
s and Task
values are different types in sbt, and there are different situations for using either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps:
buildInfoKeys in Test := BuildInfoKey.fromKeys(name, fullClasspath in Compile)
and
buildInfoKeys in Test ++= BuildInfoKey.fromKeys(name, fullClasspath in Compile)
596721d
to
3205c7d
Compare
sbt 1 requires java 8+ where MaxPermSize is no longer present.
These exist to avoid having to explain the concept of the .taskValue method and sbt.Task to users.
Introduce BuildInfoKey.outOfGraphTaskExecution as a fallback.
@eed3si9n have a look, and let me know what you think now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM
minor comment about outOfGraph..
def of(x: Any): BuildInfoKey = macro BuildInfoKeyMacros.ofImpl | ||
def ofN(xs: Any*): Seq[BuildInfoKey] = macro BuildInfoKeyMacros.ofNImpl | ||
|
||
def outOfGraphTaskExecution[A](key: TaskKey[A]): Entry[A] = Task(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe shorter name like outOfGraph(..)
? Also is this tested ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with the more verbose name, to both make the intention explicit and to discourage usage.
It's tested via the identical deprecated implicit conversion. Is that ok? Or do you want me to switch to this one? Or do you want me to add both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eed3si9n wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outOfGraphUnsafe
?
.. also upgrade to sbt 1.1.0 & cleanup.