sbt / sbt

sbt, the interactive build tool
https://scala-sbt.org
Apache License 2.0
4.81k stars 937 forks source link

Unable to reference a key scoped to the sbt-web Assets configuration in the console #4065

Open steinybot opened 6 years ago

steinybot commented 6 years ago

steps

build.sbt

enablePlugins(SbtWeb)

val foo = taskKey[Classpath]("foo")

foo := (Assets / exportedProducts).value

project/plugins.sbt

addSbtPlugin("com.typesafe.sbt" %% "sbt-web" % "1.4.3")

problem

I am unable to reference a key that is scoped to the Assets configuration from the console.

→ sbt
[info] Loading settings from idea.sbt ...
[info] Loading global plugins from /Users/jason/.sbt/1.0/plugins
[info] Loading settings from plugins.sbt ...
[info] Loading project definition from /source/sbt-config-key-bug/project
[info] Loading settings from build.sbt ...
[info] Set current project to sbt-config-key-bug (in build file:/source/sbt-config-key-bug/)
[info] sbt server started at local:///Users/jason/.sbt/1.0/server/3318aa2bd772e7169d56/sock
sbt:sbt-config-key-bug> inspect Assets / exportedProducts
[info] No entry for key.
[info] Description:
[info]  Build products that go on the exported classpath.
[info] Delegates:
[info]  Assets / exportedProducts
[info]  exportedProducts
[info]  ThisBuild / Assets / exportedProducts
[info]  ThisBuild / exportedProducts
[info]  Zero / Assets / exportedProducts
[info]  Global / exportedProducts
[info] Related:
[info]  Web-assets-test / exportedProducts
[info]  Test / exportedProducts
[info]  Runtime / exportedProducts
[info]  Web-assets / exportedProducts
[info]  Compile / exportedProducts
sbt:sbt-config-key-bug> inspect foo
[info] Task: scala.collection.Seq[sbt.internal.util.Attributed[java.io.File]]
[info] Description:
[info]  foo
[info] Provided by:
[info]  ProjectRef(uri("file:/source/sbt-config-key-bug/"), "sbt-config-key-bug") / foo
[info] Defined at:
[info]  /source/sbt-config-key-bug/build.sbt:5
[info] Dependencies:
[info]  Web-assets / exportedProducts
[info] Delegates:
[info]  foo
[info]  ThisBuild / foo
[info]  Global / foo
sbt:sbt-config-key-bug> inspect Web-assets / exportedProducts
[error] Expected project ID
[error] Expected configuration
[error] Expected ':'
[error] Expected key
[error] Not a valid key: Web-assets (similar: assets)
[error] inspect Web-assets / exportedProducts
[error]                   ^
sbt:sbt-config-key-bug> inspect web-assets / exportedProducts
[error] Expected project ID
[error] Expected configuration
[error] Expected ':'
[error] Expected key
[error] Not a valid key: web-assets (similar: assets, webJars, webStage)
[error] inspect web-assets / exportedProducts
[error]                   ^
sbt:sbt-config-key-bug>

expectation

I should be able to reference a key that is scoped to the Assets configuration from the console using either Assets, Web-assets or web-assets.

notes

sbt version: 1.1.2 gitter discussion: https://gitter.im/sbt/sbt?at=5ac4b6bf7685a046389d4c35

dwijnand commented 6 years ago

interesting. looks like a bug in the parser, maybe?

eed3si9n commented 6 years ago

https://github.com/sbt/sbt-web/blob/1.4.3/src/main/scala/com/typesafe/sbt/web/SbtWeb.scala says:

object Import {

  val Assets = config("web-assets")
  val TestAssets = config("web-assets-test").extend(Assets)
  val Plugin = config("web-plugin")
  ...

In sbt 0.13 these would be addressed as:

> show web-assets:assets

in shell, and

sourceGenerators in Assets += mySourceFileTask.taskValue

in build.sbt. inspect certainly thinks Web-assets / exportedProducts is a thing, so yeah parser is suspicious.

Note for potential contributors. The place to look would be any files involved in https://github.com/sbt/sbt/pull/3434, but especially main/src/main/scala/sbt/internal/Act.scala and main/src/main/scala/sbt/internal/KeyIndex.scala.

steinybot commented 6 years ago

I'll have a go at fixing this.

I have noticed that sbt.internal.TestBuild#idGen does not produce all the same values that is permitted by sbt.internal.util.complete.Parsers#ID. I have managed to get the test to fail when I use all "letters, digits, dash -, and underscore _".

dwijnand commented 6 years ago

I have noticed that sbt.internal.TestBuild#idGen does not produce all the same values that is permitted by sbt.internal.util.complete.Parsers#ID. I have managed to get the test to fail when I use all "letters, digits, dash -, and underscore _".

nice.

I was recently in that general area and found it unfortunate that some shared source of truth can't be used between sbt's completion parsers and scalachecks generators.

steinybot commented 6 years ago

I was recently in that general area and found it unfortunate that some shared source of truth can't be used between sbt's completion parsers and scalachecks generators.

Yeah that would be nice. I think it can only go so far though. The individual character classes could be shared but the full rule for a string would be a bit too difficult to share, especially since the test will fail if the generator filters out values. It's not the most efficient method but we could precompute the set of characters for each class based on whether the parser accepts them. I probably won't end up doing this.

Changing the generator wasn't as trivial as I thought. It is used in a bunch of places but they have different rules. I had to create separate ones. I think I finally have them correct now.

I have also been working on a Shrink[StructureKeyMask] as the failed output was rather unweildy and it helped me figure out where the test data comes from. It's almost there except that I need to fix the delegates. I'm not sure if the shrinking was disabled for a good reason or just because no one got around to writing a shrinker for it.

I'm not too sure if the failure I'm seeing is the same as the bug reported here. For some reason the configuration in the Key string has a different case than the ScopedKey. The output is a bit confusing because sbt.Scope#display always capitalises it.

dwijnand commented 6 years ago

I'm not sure if the shrinking was disabled for a good reason or just because no one got around to writing a shrinker for it.

no idea either, but I'm excited to see what you've done (I only recently learnt about Scalacheck's Shrink).

steinybot commented 6 years ago

I suspect the issue here has something to do with guessing of the configuration id. When the id is not the name decapitalized it seems to fail.

The comment on sbt.internal.ConfigIndex#fromConfigIdent says:

// guess Configuration name from an identifier. // There's a guessing involved because we could have scoped key that Project is not aware of.

In one of the tests I have, the project is (or should be) aware of the configuration and so should be able find the correct id but it is missing from the index.

steinybot commented 6 years ago

I've made some progress on this. https://github.com/steinybot/sbt/tree/fix/3432

I have improved the index but it only works reliably for configurations that come from the project and not for any that are added via other known ScopedKey's. See sbt.internal.Load#structureIndex for how it builds the KeyIndex.

For any configuration that is added via a known ScopedKey, only the name is known and so it can only be resolved the if the guessed id (capitalised name) is correct. It makes me think that the ConfigKey ought to be based on the id and not the name but I'm not sure if this is possible.

The "Related" keys are also incorrect. I haven't looked into this but it seems to be using the same guesses to the id.

There might actually be a simple workaround, which is to change SbtWeb to override sbt.AutoPlugin#projectConfigurations.

steinybot commented 6 years ago

I can confirm that this can be worked around by adding the configuration to the project.

project/SbtWebFix.scala

import sbt.AutoPlugin
import com.typesafe.sbt.web.Import

object SbtWebFix extends AutoPlugin {

  override def projectConfigurations = Seq(Import.Assets)

  override def trigger = allRequirements
}
steinybot commented 6 years ago

A simpler workaround:

build.sbt

configs(com.typesafe.sbt.web.Import.Assets)
steinybot commented 6 years ago

So this isn't really an issue with the parser. It is doing its best based on the information it has. The problem is that it only knows the configuration name.

It will work if:

  1. The configuration is defined in the build via config (there may be more restrictions around this that I haven't explored), or
  2. Explicitly added to the project with configs/overrideConfigs, or
  3. Via an AutoPlugin which itself overrides projectConfigurations, or
  4. We get lucky and the configuration id (name of the variable it is assigned to) is the same the name with the first letter in upper case.
steinybot commented 6 years ago

Something I still need to look into is how the resolution differs between a key in a command and a key in the build.

In the SbtWeb example, the setting is resolved when the build is loaded but not when used in a command. This example is a little surprising:

build.sbt

// This works
foo := (Assets / exportedProducts).value
// This doesn't work (as expected but the error message is surprising)
foo := (Assets / foo).value
[info] Loading settings from build.sbt ...
[error] Reference to undefined setting:
[error]
[error]   Web-assets / foo from foo (/source/sbt-config-key-bug/build.sbt:9)

The error message contains the incorrectly guessed configuration id although this may just be a difference between the error message and what it actually uses to resolve the key since the first one ought to fail if it was caused by a bad guess.

steinybot commented 6 years ago

The resolution for a ScopedKey works in the build because it resolves it against the map of other ScopedKey's and so it succeeds even though the ConfigKey contains the name rather than the id. This is done in sbt.internal.util.Init#delegateForKey.

The error message is using sbt.Scope#display to display the referenced key which guessed the id.

There are a few options:

  1. Do nothing more. Plugin authors can override projectConfigurations or users can add the configuration via configs.
  2. Add a warning when we encounter ScopedKey that contains a configuration axis that is not in the project's configurations. Same workaround as 1.
  3. Change ConfigKey to include the id and the name. Maintaining backwards compatibility will mean making a few guesses for the id which is no worse than what it does now. Should solve the problem reported here since the known keys in the project settings are likely to come from a sbt.librarymanagement.Configuration which does have the id.
  4. Allow configuration names to be used when parsing keys.
steinybot commented 6 years ago

Some more information on this one. This issue must have already been known to some degree, although perhaps not the full impact, since there are hard coded workarounds for three lucky configurations.

sbt.Scope#configIdents:

  private[sbt] val configIdents: Map[String, String] =
    Map(
      "it" -> "IntegrationTest",
      "scala-tool" -> "ScalaTool",
      "plugin" -> "CompilerPlugin"
    )

I had seen this earlier but I didn't appreciate what it was actually doing at the time.

steinybot commented 6 years ago

Back to the options:

  1. Is not possible without breaking backwards compatibility. ConfigKey is in the public API and support for the old key format that used the config name would have to be dropped.
  2. Is also not possible since it introduces ambiguities between configurations and tasks (e.g. compile).
  3. Kind of sucks and is what lead me down this big rabbit hole to begin with. It would be my last choice.
  4. This seems possible with the changes I have made to the ConfigIndex. At the end of sbt.internal.Load#structureIndex we just look for any config that doesn't have an identity and which also doesn't exist in configIdents.
ches commented 6 years ago

I think this is resolved since #4231?

eed3si9n commented 6 years ago

Yes. I am going to close this.

dwijnand commented 5 years ago

The "attempt to fix #4065" and "try to fix #4065" messages in 780ca366d8b7bd2a946057217d0c801d11d43b71 erroneously closed this.

eatkins commented 5 years ago

@steinybot can you elaborate on issue 3)? Naively, I think we can update ConfigKey without breaking backwards binary compatibility:

sealed case class ConfigKey(name: String) {
  def id: String
}

We would just have to drop the final keyword, but sealing the class has a similar effect to final for end users. Is there something I'm missing?