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.

Sign up for GitHub

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

Jump to bottom

Setup-ide should run before compile, run and test #188

Merged
merged 16 commits into from
Oct 15, 2021
Merged

Setup-ide should run before compile, run and test #188

merged 16 commits into from
Oct 15, 2021

Conversation

MarcinAman
Copy link
Contributor

No description provided.

@MarcinAman MarcinAman marked this pull request as ready for review October 5, 2021 15:23
@MarcinAman MarcinAman marked this pull request as draft October 5, 2021 15:24
@MarcinAman MarcinAman marked this pull request as ready for review October 7, 2021 11:35
alexarchambault
alexarchambault reviewed Oct 7, 2021
View reviewed changes
modules/cli/src/main/scala/scala/cli/commands/BspOptions.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/BspOptions.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/Compile.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/CoursierOptions.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/Run.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/SetupIde.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/Test.scala Outdated Show resolved Hide resolved
modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala Show resolved Hide resolved
modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala Outdated Show resolved Hide resolved
modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala Outdated

val importPprintOnlyProject = TestInputs(
Seq(
os.rel / "simple.sc" -> s"import $$ivy.`com.lihaoyi::pprint:${Constants.pprintVersion}`"
)
)

test("setup-ide should have only absolute path if relative one was specified") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this tests. Plus the handling of the os.Paths looks suspicious (all os.Paths are absolute and normalized).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the issue that i wanted to solve is whenever i run something like scala-cli setup-ide ../other/folder -S 3.0.1 in the directory that is not within a project then the scala-cli.json contains relative path that i posted. This is wrong as the paths (if we want relative one) should be resolved from the workspace not from the place where i run scala-cli. Therefore i thought that it would be safe to always include absolute paths just like we do when path to scala-cli is relative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be exact this is what scala-cli outputs:

{
  "name": "sbt",
  "argv": [
    "scala-cli",
    "bsp",
    "../example/scala3",
    "-S",
    "3.0.1",
    "--bsp-name",
    "sbt"
  ],
  "version": "0.0.4-SNAPSHOT",
  "bspVersion": "2.0.0-M13",
  "languages": [
    "scala",
    "java"
  ]
}⏎

When i run from: ~/Documents/scala-cli and have an example project in ~/Documents/example/scala3

Copy link
Contributor

Choose a reason for hiding this comment

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

(Weird, I thought I had answered this in an earlier review, but I can't see my answers here… Anyway, that was a good point ^^)

@MarcinAman MarcinAman force-pushed the setup-ide-before-all branch 4 times, most recently from 82d6a9e to b69886d Compare October 12, 2021 09:56
alexarchambault
alexarchambault reviewed Oct 12, 2021
View reviewed changes
modules/cli/src/main/scala/scala/cli/commands/Bsp.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/BspOptions.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/SetupIde.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/SetupIde.scala Outdated
os.FilePath(rawArgv(0)).resolveFrom(Os.pwd).toString
else {
val absoluteScalaCliPath =
this.getClass.getProtectionDomain.getCodeSource.getLocation.toURI.getPath.trim
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is going to work from the native executables… (it should throw an NPE I think).

Using just rawArgv(0) should be fine here (these would be even better, but that can wait…)

modules/cli/src/main/scala/scala/cli/commands/SetupIde.scala Outdated Show resolved Hide resolved
modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala Outdated

val importPprintOnlyProject = TestInputs(
Seq(
os.rel / "simple.sc" -> s"import $$ivy.`com.lihaoyi::pprint:${Constants.pprintVersion}`"
)
)

test("setup-ide should have only absolute path if relative one was specified") {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Weird, I thought I had answered this in an earlier review, but I can't see my answers here… Anyway, that was a good point ^^)

modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala Outdated Show resolved Hide resolved
modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala Outdated Show resolved Hide resolved
@MarcinAman MarcinAman force-pushed the setup-ide-before-all branch 5 times, most recently from 050e568 to dd566d8 Compare October 13, 2021 14:03
alexarchambault
alexarchambault reviewed Oct 13, 2021
View reviewed changes
modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala Outdated
@@ -125,6 +126,17 @@ abstract class BspTestDefinitions(val scalaVersionOpt: Option[String])
expect(expectedPrefixes.exists(uri.startsWith))
}

def readBspConfig(root: os.Path, f: (Details) => Unit): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT Why not just returning the Details? And let the tests below do whatever they want with them?

modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala Outdated Show resolved Hide resolved
@MarcinAman MarcinAman force-pushed the setup-ide-before-all branch 3 times, most recently from 305a3fb to 9e9262b Compare October 14, 2021 14:46
MarcinAman and others added 9 commits October 14, 2021 16:49
@MarcinAman @alexarchambault
Setup-ide should run before compile, run and test
c67af3f 
Co-authored-by: Alexandre Archambault <alexarchambault@users.noreply.github.com>

Co-authored-by: Alexandre Archambault <alexarchambault@users.noreply.github.com>
@MarcinAman
Rewrite imports
d962e51
@MarcinAman
Absolute paths, ide-options.json and runSafe
f1e596d
@MarcinAman @alexarchambault
Apply suggestions from code review
9f1760b 
Co-authored-by: Alexandre Archambault <alexarchambault@users.noreply.github.com>
@MarcinAman
Try making scala-cli in bsp absolute, bring back test
eea126e
@MarcinAman
Greedy algorithm for dropping urls
33b0f19
@MarcinAman @alexarchambault
Update modules/integration/src/test/scala/scala/cli/integration/BspTe…
f11e155 
…stDefinitions.scala

Co-authored-by: Alexandre Archambault <alexarchambault@users.noreply.github.com>
@MarcinAman
Make paths absolute if they exist
316574a
@MarcinAman
Add coursier.mainJar and fallback to current class location
94611a1
@alexarchambault
Copy link
Contributor

alexarchambault commented Oct 14, 2021 •
edited
Loading

Just pushed a NIT commit, with mostly minor syntax changes or dummy refactorings. Otherwise, LGTM.

@alexarchambault
fixup
1896c37
@alexarchambault
Don't run setup-ide for virtual inputs
1fc7abe 
And don't re-parse the input arguments, just use what's in the Inputs
instance.
@alexarchambault
Update tests
28048fb
@alexarchambault
fixup
dea12de
@alexarchambault
fixup
c7302c3
@alexarchambault
fixup
da559f4
@alexarchambault
Copy link
Contributor

CI is green 🎉

@alexarchambault alexarchambault merged commit 5cd81f5 into VirtusLab:master Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexarchambault alexarchambault Awaiting requested review from alexarchambault

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MarcinAman @alexarchambault