improvement : Able to get environment variables from shell #7301

ajafri2001 · 2025-03-13T21:27:11Z

Co-authored-by : Tomasz Godzik [email protected]
Co-authored-by : Marcin Sobejko [email protected]

I want to finish this, what would the next steps look like?

tgodzik · 2025-03-14T19:26:33Z

First step would be to try and set it up for yourself to see if it works, I've also been thinking about how to test it and I think we can do a test in https://github.com/scalameta/metals/blob/main/tests/slow/src/test/scala/tests/sbt/SbtBloopLspSuite.scala#L39

You could copy one of the basic tests and add something that would fail like:

sys.env.get("MY_ENV_VAR").get

to build.sbt file in the test.

Later we can create a fake bash, which would add an export for MY_ENV_VAR and invoke normal bash. We the use the new defaultShell setting like in

metals/tests/unit/src/main/scala/tests/BuildServerInitializer.scala

Line 86 in 52309be

_ <- server.didChangeConfiguration(userConfig.toString())

but we would do copy on the default userConfig.copy(defaultShell = Some(fakepath))

After that it should not fail to import the sbt definition into bloop.

Let me know if you get stuck!

ajafri2001 · 2025-03-25T10:28:56Z

I have not deserted this 😄.

Co-authored-by : Tomasz Godzik <[email protected]> Co-authored-by : Marcin Sobejko <[email protected]>

… before

tgodzik · 2025-03-31T14:30:42Z

metals/src/main/scala/scala/meta/internal/builds/ShellRunner.scala

@@ -87,15 +92,22 @@ class ShellRunner(time: Time, workDoneProvider: WorkDoneProgress)(implicit
  ): Future[Int] = {
    val elapsed = new Timer(time)
    val env = additionalEnv ++ JdkSources.envVariables(javaHome)
+
+    val shellArguments = userConfiguration().defaultShell match {
+      case Some(shell) => List(shell, "-i", "-l", "-c", args.mkString(" "))


would all shells have the same options though?

I really do think so. I've tried bash, zsh and fish so far, and they all seem to work with the above flags. If some obscure shell doesn't work with some of the flags, we should be able to adjust them like this.

val shellArguments = userConfiguration().defaultShell match { case Some(shell) => shell match { case "/usr/bin/fish" => List(shell, "-c", args.mkString(" ")) case _ => List(shell, "-i", "-l", "-c", args.mkString(" ")) } case None => args }

what do you think?

People might have the fish shell located somewhere else, so this wouldn't work. Do we need the additional options though?

Suggested change

case Some(shell) => List(shell, "-i", "-l", "-c", args.mkString(" "))

case Some(shell) => shell :: args

should work, no?

OR you could do Some(shell) if shell.contains("fish") to be on the safe side.

We should also pack it with Try() and return an empty map if there is an exception

tgodzik · 2025-03-31T14:31:53Z

tests/slow/src/test/scala/tests/sbt/SbtBloopLspSuite.scala

+            |/build.sbt
+            |scalaVersion := "${V.scala213}"
+            |sys.env.get("MY_ENV_VAR").get
+            |""".stripMargin


You can create a bash script here and provide that as different defaultShell

I think I'm stuck here and need help 😓. I'm not sure how the fake-shell test is supposed to work, but I pushed a draft test I put together for you to take a look at whenever you have the time.

Looks good, though you might need to add executable flag to fake-shell.sh

We should add:
_ <- server.executeCommand(ServerCommands.ImportBuild) at the end and it it doesn't fail we should be good.

tgodzik

Sorry for getting back so late to it!

tgodzik · 2025-04-09T13:34:58Z

metals/src/main/scala/scala/meta/internal/builds/ShellRunner.scala

@@ -87,15 +92,22 @@ class ShellRunner(time: Time, workDoneProvider: WorkDoneProgress)(implicit
  ): Future[Int] = {
    val elapsed = new Timer(time)
    val env = additionalEnv ++ JdkSources.envVariables(javaHome)
+
+    val shellArguments = userConfiguration().defaultShell match {
+      case Some(shell) => List(shell, "-i", "-l", "-c", args.mkString(" "))


People might have the fish shell located somewhere else, so this wouldn't work. Do we need the additional options though?

Suggested change

case Some(shell) => List(shell, "-i", "-l", "-c", args.mkString(" "))

case Some(shell) => shell :: args

should work, no?

tgodzik · 2025-04-09T13:35:17Z

metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala

@@ -455,6 +456,7 @@ object UserConfiguration {
           |default to using it instead of Bloop.
           |""".stripMargin,
      ),
+      UserConfigurationOption("default-shell", "", "", "", ""),


Let's make sure we explain the option properly

tgodzik · 2025-04-09T13:37:44Z

tests/slow/src/test/scala/tests/sbt/SbtBloopLspSuite.scala

+            |/build.sbt
+            |scalaVersion := "${V.scala213}"
+            |sys.env.get("MY_ENV_VAR").get
+            |""".stripMargin


Looks good, though you might need to add executable flag to fake-shell.sh

We should add:
_ <- server.executeCommand(ServerCommands.ImportBuild) at the end and it it doesn't fail we should be good.

tgodzik · 2025-04-09T13:41:32Z

tests/slow/src/test/scala/tests/sbt/SbtBloopLspSuite.scala

+    workspace.resolve("sbt").createDirectories()
+
+    for {
+      _ <- initialize(


Ach and initialize itself will fail, but you can use recover{ case t => } to make sure the correct exception is being thrown. That's why the tests now fail, it tries to import automatically and it does fail, which is expected.

ajafri2001 · 2025-04-17T11:53:01Z

Sorry, for the late reply, I was traveling after wrapping up my coursework for the semester 😵.

 - case Some(shell) => List(shell, "-i", "-l", "-c", args.mkString(" "))
 + case Some(shell) => shell :: args

I did this because of the following observation, if I have an env variable in my ~/.bashrc and my current shell is zsh, If I type /usr/bin/bash env | grep ARTIFACTORY_USERNAME | wc -l to grep for the env variable, it doesn't work, I'd have to change it and add flags like /usr/bin/bash -i -l -c "env | grep ARTIFACTORY_USERNAME | wc -l" for it to work.

An example using fish shell, for which I placed the env variable under config.fish

❯ /usr/bin/fish env | grep ARTIFACTORY_USERNAME | wc -l
error: Error reading script file 'env':
No such file or directory (os error 2)
0
❯ /usr/bin/fish -i -l -c "env | grep ARTIFACTORY_USERNAME | wc -l"                                                                                                                                                  
1

Note - This is all done via heuristics and informal reasoning 😄 and there actually might be a simpler solution, For me, the code change wasn't working until I added all the flags.

ajafri2001 · 2025-04-27T11:53:18Z

Hi @tgodzik, whenever you have some time, would you mind taking a look at this? 🙂 I was also wondering where it might make sense to include a try block. 🤔 The metals.log file already provides very helpful error messages if the user messes up, so I'm not sure if it's too necessary. Thanks a lot! 🥳

improvement : Able to get environment variables from vscode

552b0a3

Co-authored-by : Tomasz Godzik <[email protected]> Co-authored-by : Marcin Sobejko <[email protected]>

ajafri2001 force-pushed the issue-7252 branch from 4d32416 to 552b0a3 Compare March 26, 2025 16:42

ajafri2001 added 3 commits March 27, 2025 00:53

Stuff runs - env variables are being loaded properly

81f8323

remove log statements and add comments

88a17ca

bugfix : Incase user doesn't provide defaultShell, do how it was done…

d21f0c3

… before

ajafri2001 changed the title ~~improvement : Able to get environment variables from vscode~~ improvement : Able to get environment variables from shell Mar 27, 2025

tgodzik reviewed Mar 31, 2025

View reviewed changes

ajafri2001 and others added 4 commits April 1, 2025 03:10

Merge branch 'scalameta:main' into issue-7252

03038d5

added bash-script in the test

4ba45e2

Merge branch 'scalameta:main' into issue-7252

841544a

Merge branch 'scalameta:main' into issue-7252

59139c7

tgodzik reviewed Apr 9, 2025

View reviewed changes

ajafri2001 and others added 3 commits April 15, 2025 17:21

Merge branch 'scalameta:main' into issue-7252

0e243c7

Added user-configuration-options description

e5b014e

fix : Scalafix

86c88b8

ajafri2001 marked this pull request as ready for review April 18, 2025 15:31

ajafri2001 added 2 commits April 26, 2025 18:53

fix : resolved merge conflicts

0ba7016

fix : Wrote correct test

3c5a843

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement : Able to get environment variables from shell #7301

improvement : Able to get environment variables from shell #7301

ajafri2001 commented Mar 13, 2025

tgodzik commented Mar 14, 2025

ajafri2001 commented Mar 25, 2025

tgodzik Mar 31, 2025

ajafri2001 Apr 1, 2025

tgodzik Apr 9, 2025

tgodzik Apr 25, 2025

tgodzik Apr 25, 2025

tgodzik Mar 31, 2025

ajafri2001 Apr 2, 2025

tgodzik Apr 9, 2025

tgodzik left a comment

tgodzik Apr 9, 2025

tgodzik Apr 9, 2025

tgodzik Apr 9, 2025

tgodzik Apr 9, 2025

ajafri2001 commented Apr 17, 2025 •

edited

Loading

ajafri2001 commented Apr 27, 2025

	case Some(shell) => List(shell, "-i", "-l", "-c", args.mkString(" "))
	case Some(shell) => shell :: args

improvement : Able to get environment variables from shell #7301

Are you sure you want to change the base?

improvement : Able to get environment variables from shell #7301

Conversation

ajafri2001 commented Mar 13, 2025

tgodzik commented Mar 14, 2025

ajafri2001 commented Mar 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafri2001 commented Apr 17, 2025 • edited Loading

ajafri2001 commented Apr 27, 2025

ajafri2001 commented Apr 17, 2025 •

edited

Loading