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

use directives instead of scalac options in tests #18560

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

LydiaSkuse
Copy link
Contributor

@LydiaSkuse LydiaSkuse commented Sep 15, 2023

  • update remaining // scalac: to //> using options in tests
  • throw error when using old syntax in tests

fixes scalac: portion of #18149
still to do:

  • rewrite // test: -jvm 15+ to use a directive
  • support // scalajs: --skip with a directive or similar

once this is done, we can remove the toolArg regex

@SethTisue
Copy link
Member

SethTisue commented Sep 15, 2023

LGTM, but yeah let's try and get rid of the remaining usages — doesn't have to be in this PR though, this much is mergeable on its own.

as for the remaining usages,

// scalajs: --skip

could be //> using platform jvm

Jamie points out "what about native", but I don't think that's our problem, JVM and JS are the only platforms in play in this repo, and anyway it could always be adjusted later

// test: -jvm 15+

could be //> using jvm 15+

Scala-CLI doesn't know the "15+" syntax (I don't think), but that's fine, having it be so similar will still aid our memories and will still facilitate moving test code back and forth between vulpix and scala-cli.

@LydiaSkuse
Copy link
Contributor Author

nice, thanks. will follow up with another PR.

i have also noticed

  • // scalajs: --compliant-semantics - not sure how to replace this
  • /* javac: -encoding UTF-8 */ - doesn't seem to be used, test still passes without the whole thing

@SethTisue
Copy link
Member

// scalajs: --compliant-semantics - not sure how to replace this

@sjrd?

/* javac: -encoding UTF-8 */ - doesn't seem to be used, test still passes without the whole thing

Could fail on some OS/JVM combinations — prior to JDK 18, the default encoding on Windows wasn't UTF-8.

Anyway, the Scala-CLI equivalent is //> using javacOpt ...

@sjrd
Copy link
Member

sjrd commented Sep 15, 2023

I don't think scala-cli has an option for compliant semantics. It's still missing a number of linking config items.

@som-snytt
Copy link
Contributor

Although I'm glad to see support for scala-cli directive, also sad to see everything has to be a scala-cli directive.

I would anticipate that tests need more config than cli, although I hesitate to explore that problem space.

I see previous comments cover (random grep)

tests/run/t12348.scala:// test: -jvm 11+

I don't know the cli syntax, but do I want to know?

@SethTisue
Copy link
Member

SethTisue commented Sep 16, 2023

also sad to see everything has to be a scala-cli directive

if you get nostalgic for the old syntax you can always get your fix over in the scala/scala repo.

if scala-cli doesn't have a directive for something, it is arguably odd to make up something that looks like a scala-cli directive even though scala-cli wouldn't actually accept it. regardless, I think we should have one syntax, even if it means we have to invent a couple of nonstandard directives for use in this repo only

there was some talk around the tooling summit about reserving //> using ... for Scala-CLI but allowing/encouraging other tools to use //> ammonite ... and //> vulpix ... and so forth for tool-specific directives. @lefou if I'm not mistaken that discussion is only in a private repo at the moment and if so, maybe we could surface it somewhere? (UPDATE: oh I see you did talk about it at com-lihaoyi/Ammonite#1372 (comment))

@lefou
Copy link

lefou commented Sep 16, 2023

also sad to see everything has to be a scala-cli directive

if you get nostalgic for the old syntax you can always get your fix over in the scala/scala repo.

if scala-cli doesn't have a directive for something, it is arguably odd to make up something that looks like a scala-cli directive even though scala-cli wouldn't actually accept it. regardless, I think we should have one syntax, even if it means we have to invent a couple of nonstandard directives for use in this repo only

there was some talk around the tooling summit about reserving //> using ... for Scala-CLI but allowing/encouraging other tools to use //> ammonite ... and //> vulpix ... and so forth for tool-specific directives. @lefou if I'm not mistaken that discussion is only in a private repo at the moment and if so, maybe we could surface it somewhere? (UPDATE: oh I see you did talk about it at com-lihaoyi/Ammonite#1372 (comment))

It would be cool to use a "using directive", but apply a different more appropiate context to it, like //> dotty-test options.

I wrote up that idea in the organizational project used for the Scala Tooling Summits (https://github.com/scalacenter/tooling-working-group), but I didn't receive any feedback so far. I'm glad to copy / re-write it somewhere else for a greater audience. Do you have a suggestion where, @SethTisue ?

@dwijnand
Copy link
Member

The risk of using "non standard" directives is that you might end up with the same directive implemented with different semantics. Like how sbt-extras and its clone have different semantics for .jvm-opts lines... I rather keep things that aren't officially supported as something else that isn't a scala cli use directive, as they are.

@lefou
Copy link

lefou commented Sep 17, 2023

The risk of using "non standard" directives is that you might end up with the same directive implemented with different semantics. Like how sbt-extras and its clone have different semantics for .jvm-opts lines... I rather keep things that aren't officially supported as something else that isn't a scala cli use directive, as they are.

Hence my suggestion, to keep the syntax (same tooling to extract directives) but alter the context (different semantics).

//> using myoption=value1
//> dotty-test myoption=value1

Those two lines aren't the same directives. The first belongs to Scala-CLI and the second to the test suite of the dotty project (this one). The semantics are defined by the context.

@sjrd
Copy link
Member

sjrd commented Sep 17, 2023

During the tooling summit, it seems to me that we had consensus that, for this kind of use cases, we should use

//> using dottytest.myoption value1

@lefou
Copy link

lefou commented Sep 17, 2023

During the tooling summit, it seems to me that we had consensus that, for this kind of use cases, we should use

//> using dottytest.myoption value1

Hm, that doesn't make clear which tools is supposed to interpret it correctly though. Is there anywhere a write up of that discussion / consensus?

@sjrd
Copy link
Member

sjrd commented Sep 17, 2023

Not yet. It was just last week and we directly segue'ed into Scala Days.

@som-snytt
Copy link
Contributor

Airplane travel is designed for writing things up. When there is more to write up, they still have rail.

In the Star Trek universe, they realized that transporter technology would have a devastating effect on documentation, because the segue to Scala Days would be effectively instantaneous, so they started recording holographic reconstructions of all missions. It's all stored in the 23rd century equivalent of git. That is why 23rd-century code reviews are merciless.

Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again Lydia! I think those follow ups can be done in a separate PR

@bishabosha bishabosha merged commit 090710a into scala:main Sep 28, 2023
17 checks passed
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
…20627)

Backports #18560 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
…20684)

Backports #18560 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants