Skip to content
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

Defining project.scala dependencies in test scope? #1729

Closed
francisdb opened this issue Dec 23, 2022 · 12 comments · Fixed by #2046
Closed

Defining project.scala dependencies in test scope? #1729

francisdb opened this issue Dec 23, 2022 · 12 comments · Fixed by #2046
Assignees
Labels
bug Something isn't working

Comments

@francisdb
Copy link

Version(s)
Scala CLI version: 0.1.19
Scala version (default): 3.2.1

I see this warning on every compile

[warn] ./files.test.scala:1:1: Using directives detected in multiple files. It is recommended to keep them centralized in the /xyz/project.scala file.

But was wondering how I define dependencies in test scope inside project.scala or is there no such thing as a test scope?
I have not found anything related to that in the docs?

@Gedochao Gedochao added the bug Something isn't working label Jan 4, 2023
@Gedochao
Copy link
Contributor

Gedochao commented Jan 4, 2023

Hey, sorry for the late answer. There indeed should be a way to define test scope directives in the configuration file, but there currently isn't. It's definitely a bug, and it hasn't been covered in the docs properly, as you say.

All sources, including the project.scala configuration file, should respect being in the test scope, as described in our test command doc. The warning doesn't seem to take it into account.

I know it may be annoying, but it seems the best I can advise is to ignore the warning until we fix this.
As things are, the warning will only go away if you keep all your directives in a single source file, which probably isn't ideal, as you probably wouldn't want your test dependencies in the main scope.

@prolativ
Copy link

prolativ commented Jan 4, 2023

There is a similar problem when one needs a different set of directives when cross-compiling to different targets: JVM/native/js and different scala version

@tgodzik
Copy link
Member

tgodzik commented Jan 4, 2023

We are thinking of having a short term solution to disable the warning, but long term we should have somethin like:
//> using test.lib ... which would mean we are adding the library only to the test scope. Similar with native/js/jvm

@Gedochao
Copy link
Contributor

Gedochao commented Jan 5, 2023

We are thinking of having a short term solution to disable the warning

That can be tracked under a separate issue: #1747

@Gedochao
Copy link
Contributor

Gedochao commented Mar 1, 2023

We are thinking of having a short term solution to disable the warning, but long term we should have somethin like: //> using test.lib ... which would mean we are adding the library only to the test scope. Similar with native/js/jvm

It's been decided we will proceed with this syntax, as in the prefix test.* will indicate a directive should only affect the test scope.
no separate project file will be used. (like project.test.scala)
example:

//> using test.dep "(...)"

@prolativ
Copy link

prolativ commented Mar 2, 2023

This might solve the problem for test/main and potentially for jvm/js/native. But what if one e.g. needs different library dependencies for scala 2.12, 2.13 and 3? Would it be

//> using scala.2_12.dep "..."
//> using scala.2_13.dep "..."
//> using scala.3.dep "..."

?
This might cover some most common use cases, however:

  1. Would a directive key like scala.2_12.dep in some way clash with the scala key?
  2. How would one simulate the semantics of more complex cases like
    //> using target.scala.< "2.13"
    //> using target.platform "js"
    //> using dep "..."
    ?
  3. We would have another (third, I guess) category of directives:
    a) specifying a setting for all source files in the same build scope (with the same value of target.{scope,scala,platform})
    b) specifying, to which build scope the current file belongs
    c) (new) - like a) but applied conditionally only to some other (more specific?) build scope
  4. how should we handle cases like
    //> using target.scope "test"
    //> using main.dep "..."
    ?

@tgodzik
Copy link
Member

tgodzik commented Mar 2, 2023

This comes close to sbt territory, where it's solved programmatically. It might hard to solve this declaratively I think. Did we even plan to support it? I think we could track the different scala versions under a separate issue and not allow it at all for now.

Unless this is something we already support, if anyone can correct me here?

@prolativ
Copy link

prolativ commented Mar 3, 2023

Currently if you want to make a directive apply only to some subset of sources (e.g. only for tests) you simply have to put the directive somewhere in a file in this subset. This means that e.g. if you have a file called project.test.scala and in this file you write //> using dep "foo::bar:0.1.2" then this library will be treated as a dependency only for tests. Or instead of giving your file a specific name you could write //> using target.scope "test" or //> using target.platform "js" etc. depending on where you need other directives to apply.
To currently this works but it's problematic because it's the only way to achieve this behaviour and we advise users to put the entire build configuration of a project in a single file and raise warnings if they don't do so.

@lwronski
Copy link
Contributor

This might solve the problem for test/main and potentially for jvm/js/native. But what if one e.g. needs different library dependencies for scala 2.12, 2.13 and 3? Would it be

We don't intend to cover all scopes in this issue and will mainly focus on the Main and Test scopes as they are part of SIP. However, we acknowledge that the target directives are not well-designed and need to be reconsidered in another discussion.

I think we would should add the test.* prefix to indicate that a directive should only affect the test scope. This should be used only in project.scala to avoid any confusing warning.

If someone uses this syntax in the Main scope, it should be printed an appropriate warning indicating that test.* is designed only for use in project.scala, and that it doesn't make sense to declare Test scope options in the Main scope.

For the Test scope, it isn't necessary to declare directives with the test.* prefix. All directives declared in files from in Test scope, such as Run.test.scala, should be treated as test scope directives.

@prolativ
Copy link

Should we really try to enforce the naming of the file where the centralized configuration of a build is located?
IMO if we had Foo.scala and Foo.test.scala then putting //> suing test.dep ... in Foo.scala should be also OK as long as there's only one file in the main scope containing directives.

@tgodzik
Copy link
Member

tgodzik commented Mar 15, 2023

Should we really try to enforce the naming of the file where the centralized configuration of a build is located?
IMO if we had Foo.scala and Foo.test.scala then putting //> suing test.dep ... in Foo.scala should be also OK as long as there's only one file in the main scope containing directives.

If you have everything in a single file I think it will not generate any warning currently either, but if you have directives in both .scala and .test.scala that touch the test scope the warning should be shown.

@tgodzik
Copy link
Member

tgodzik commented Apr 12, 2023

One thing I didn't realize is that we actually want to take a different direction, where we would just have separate test directive, which wouldn't really have anything to do with scopes. This was decided while I was on vacation, so I lost a bit of context here 😓

The main difference here is that test.dep would be an entirely different directive instead of scoped one CC @romanowski

@Gedochao let's discuss it a bit more once you are back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants