sbt-native-packager icon indicating copy to clipboard operation
sbt-native-packager copied to clipboard

AshScriptPlugin generates unnecessary warnings for undefined environment variables

Open AesaKamar opened this issue 7 years ago • 20 comments

Expected behaviour

When building packages using the AshScriptPlugin and the DockerPlugin Running docker-compose up Should generate no warnings or errors from the shell

Actual behaviour

Running `docker-compose up' Actually generates

fooservice_1  | bin/fooservice: line 45: is_cygwin: not found
fooservice_1  | bin/fooservice: line 45: addJava: not found

Information

  • What sbt-native-packager are you using : 1.1.6
  • What sbt version : 0.13.15
  • What is your build system (e.g. Ubuntu, MacOS, Windows, Debian ) : macOS 10.12.4
  • What package are you building (e.g. docker, rpm, ...) : docker
  • What version has your build tool (find out with e.g. rpm --version) : Docker version 17.03.1-ce, build c6d412e
  • What is your target system (e.g. Ubuntu 16.04, CentOS 7) : Alpine Linux on docker

AesaKamar avatar May 16 '17 15:05 AesaKamar

Thanks for your report @AesaKamar

I wonder where these variables are being referenced. The ash-template doesn't use them.

Can you post the content of bin/fooservice here?

muuki88 avatar May 24 '17 20:05 muuki88

@AesaKamar I updated the test-project-docker to reproduce this, but without luck. Can you provide a small example to demonstrate this?

To test with my setup do the following

  1. checkout the current v1.1.x branch with git checkout v1.1.x
  2. cd test-project-docker
  3. sbt docker:publishLocal
  4. docker-compose up

I don't see any warnings. I'm building the image on Ubuntu 17.04 with Docker version 17.04.0-ce, build 4845c56

muuki88 avatar May 28 '17 13:05 muuki88

I'm on the same project as Aesa but I'm also not seeing these functions referenced in the ash template https://github.com/sbt/sbt-native-packager/blob/master/src/main/resources/com/typesafe/sbt/packager/archetypes/scripts/ash-template

some extra info, we're using these plugins https://github.com/cakesolutions/sbt-cake/tree/v1.0.4/src/main/scala/net/cakesolutions

of most relevance are

  • https://github.com/cakesolutions/sbt-cake/blob/v1.0.4/src/main/scala/net/cakesolutions/CakeDockerPlugin.scala
  • https://github.com/cakesolutions/sbt-cake/blob/v1.0.4/src/main/scala/net/cakesolutions/CakeJavaAppPlugin.scala

fommil avatar Jul 06 '17 12:07 fommil

oh, hold on, addJava is definitely in this script

but not in v1.1.6... hmm https://github.com/sbt/sbt-native-packager/blob/v1.1.6/src/main/resources/com/typesafe/sbt/packager/archetypes/ash-template

fommil avatar Jul 06 '17 12:07 fommil

upgrading to 1.2.0 we no longer see the addJava warning, but this still exists

myproject_1  | bin/myproject: line 56: is_cygwin: not found

fommil avatar Jul 06 '17 13:07 fommil

which is weird, because line 56 is https://github.com/sbt/sbt-native-packager/blob/v1.2.0/src/main/resources/com/typesafe/sbt/packager/archetypes/scripts/ash-template#L56

[ -f "$script_conf_file" ] && opts=$(loadConfigFile "$script_conf_file")

fommil avatar Jul 06 '17 13:07 fommil

aaah, but the generated file, line 56 looks like this

addJava "-Duser.dir=$(realpath "$(cd "${app_home}/.."; pwd -P)"  $(is_cygwin && echo "fix"))"

which looks like it is coming from ${{template_declares}}

fommil avatar Jul 06 '17 13:07 fommil

show bashScriptDefines AHA! There it is. Also in bashScriptExtraDefines

fommil avatar Jul 06 '17 13:07 fommil

my workaround is to set this in projectSettings

    bashScriptExtraDefines := List(
      """addJava "-Duser.dir=$(realpath "$(cd "${app_home}/.."; pwd -P)")""""
    )

or defining is_cygwin to return false. Probably the best way to fix this in core would be to add the is_cygwin script to this template.

fommil avatar Jul 06 '17 13:07 fommil

Thanks for the digging @fommil ❤️

Where does this line come from?

addJava "-Duser.dir=$(realpath "$(cd "${app_home}/.."; pwd -P)"  $(is_cygwin && echo "fix"))"

I couldn't find this in native-packer nor in the sbt-cake code base yet. I have searched von is_cygwin and only found the bash-template and bash-forwarder scripts.

muuki88 avatar Jul 09 '17 06:07 muuki88

Hmm, I had assumed native-packager was adding it. Is it generated somewhere? We're not adding it. Spooky.

fommil avatar Jul 09 '17 08:07 fommil

Very spooky. I tried to find the echo "fix" line as well without any luck.

muuki88 avatar Jul 09 '17 08:07 muuki88

I wonder if another plugin is adding it with reflection magic.

fommil avatar Jul 09 '17 08:07 fommil

I'm seeing it coming from https://github.com/playframework/playframework/blob/a4d9414e13ba46852e11d1cbf046703921d175bf/framework/src/sbt-plugin/src/main/scala/play/sbt/PlaySettings.scala#L223

kaylanm avatar Feb 27 '18 18:02 kaylanm

well I'm not sure but bashScriptExtraDefines should probably only add something for bash scripts - I mean it should not add anything to the ash script at all?

schmitch avatar Mar 13 '18 19:03 schmitch

@schmitch it actually does. We reused this setting. The distinction is currently between "Unix" and "windows" start scripts not between various shell implementations.

muuki88 avatar Mar 16 '18 22:03 muuki88

Hi,

As @fommil said:

Probably the best way to fix this in core would be to add the is_cygwin script to this template.

Would be a solution for the issue or the is_cygwin has bash-specific code that is not compatible with ash?

glammers1 avatar Jan 16 '19 15:01 glammers1

Thanks for taking an initiative again on this one @glammers1 :hugs: . I have no experience with ash whatsoever, but we do have some docker integration tests, so feel free to create a pull request and we see what happens :smile:

muuki88 avatar Jan 22 '19 16:01 muuki88

Hi, I have some questions about the possible solutions. The problem is not just adding the is_cygwin function to ash-template, the question is whether we should add all the functions and uses related to cygwin to ash-template as in bash-template are. For example, the function realpath is aware of cygwin in bash-template. In fact, its use in playframework is to invoke realpath where the second parameter indicates if we are using cygwin (result of the function is_cygwin). It would make no sense to use realpath with its second parameter "CHECK_CYGWIN != empty" and not change cygwin style paths to windows style paths as it would happen in ash-template.

The two possible solutions I have thought of are:

1 - ash-template is not designed to use with cygwin (docs should be updated). The solution would be add the following:

# ash-template is not designed to use with cygwin
is_cygwin() {
  return 1
}

2 - ash-template is adapted to use with cygwin. It involves adding everything related to cygwin from bash-template to ash-template.

TBH, I don't know which solution would be the right one or if there are others that I have not thought.

glammers1 avatar Jan 23 '19 14:01 glammers1

I'll heavily favor solution 1. Even I have neither used cygwin nor ash from my understanding

  • cygwin provides linux tools on windows. If you are building a docker image and want to use ash, you explicitly don't want to use cygwin
  • is there a working windows docker image so that cygwin would actually be working in a docker image?
  • windows 10 including bash and finally shipping an SSH client will probably shift cygwin out

muuki88 avatar Jan 25 '19 08:01 muuki88

@muuki88 5 years later... I agree probably just adding a is_cygwin function to the ash template returning 1 would probably be the sanest thing to do. If you are still OK with that I would provide a pull request.

mkurz avatar Mar 14 '24 13:03 mkurz

Go for it ❤️❤️

muuki88 avatar Mar 14 '24 15:03 muuki88

@muuki88 See

  • #1590

if you could take a look this would be nice.

mkurz avatar Mar 14 '24 19:03 mkurz

Thanks for merging @muuki88, this issue is now resolved and can be closed.

mkurz avatar Mar 14 '24 20:03 mkurz