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

AshScriptPlugin generates unnecessary warnings for undefined environment variables #978

Closed
AesaKamar opened this issue May 16, 2017 · 24 comments

Comments

@AesaKamar
Copy link

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
@muuki88 muuki88 added the docker label May 24, 2017
@muuki88
Copy link
Contributor

muuki88 commented May 24, 2017

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 added a commit that referenced this issue May 28, 2017
muuki88 added a commit that referenced this issue May 28, 2017
@muuki88
Copy link
Contributor

muuki88 commented May 28, 2017

@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

@fommil
Copy link

fommil commented Jul 6, 2017

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
Copy link

fommil commented Jul 6, 2017

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
Copy link

fommil commented Jul 6, 2017

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
Copy link

fommil commented Jul 6, 2017

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
Copy link

fommil commented Jul 6, 2017

show bashScriptDefines AHA! There it is. Also in bashScriptExtraDefines

@fommil
Copy link

fommil commented Jul 6, 2017

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.

@muuki88
Copy link
Contributor

muuki88 commented Jul 9, 2017

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.

@fommil
Copy link

fommil commented Jul 9, 2017

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

@muuki88
Copy link
Contributor

muuki88 commented Jul 9, 2017

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

@fommil
Copy link

fommil commented Jul 9, 2017

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

@kaylanm
Copy link

kaylanm commented Feb 27, 2018

@schmitch
Copy link
Contributor

schmitch commented Mar 13, 2018

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?

@muuki88
Copy link
Contributor

muuki88 commented Mar 16, 2018

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

@glammers1
Copy link
Contributor

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?

@muuki88
Copy link
Contributor

muuki88 commented Jan 22, 2019

Thanks for taking an initiative again on this one @glammers1 🤗 . 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 😄

@glammers1
Copy link
Contributor

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.

@muuki88
Copy link
Contributor

muuki88 commented Jan 25, 2019

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

@mkurz
Copy link
Member

mkurz commented Mar 14, 2024

@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.

@muuki88
Copy link
Contributor

muuki88 commented Mar 14, 2024

Go for it ❤️❤️

@mkurz
Copy link
Member

mkurz commented Mar 14, 2024

@muuki88 See

if you could take a look this would be nice.

@mkurz
Copy link
Member

mkurz commented Mar 14, 2024

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

@muuki88 muuki88 closed this as completed Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants