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 - Missing addResidual method in script #1264

Closed
rfranco opened this issue Oct 10, 2019 · 5 comments
Closed

AshScriptPlugin - Missing addResidual method in script #1264

rfranco opened this issue Oct 10, 2019 · 5 comments

Comments

@rfranco
Copy link
Contributor

rfranco commented Oct 10, 2019

Expected behaviour

sbt docker:publishLocal
docker run $IMAGE abc

Actual behaviour

/opt/docker/bin/script: line 79: addResidual: not found

Problem

It's missing the addResidual method in script

Information

  • What sbt-native-packager are you using
    1.4.1
  • What sbt version
    1.3.2
  • What is your build system (e.g. Ubuntu, MacOS, Windows, Debian )
    MacOs
  • 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 19.03.2, build 6a30dfc
  • What is your target system (e.g. Ubuntu 16.04, CentOS 7)
    Alpine
@muuki88
Copy link
Contributor

muuki88 commented Oct 24, 2019

Thanks @rfranco for your bug report. This looks very much like a bug. Do you have the time to fix this? I would be very happy to merge a PR for this 😍 Feel free to ask for help 😃

@archeuclid
Copy link

archeuclid commented Oct 29, 2019

Hello

I wrote some minimal code to fix the issue. Added a new plugin and associated keys with sh template. Tested on Ubuntu 18 with Scala 2.13.1. App starts with the script as well as in openjdk:8-jre-alpine Docker image. This branch issue-1264-bugfix-refactor.

Still need to add forwarder template, cygwin portability, and tests.

I suggest to define some minimal specifications to all the scripts that are produced (e.g inherit or determine java launching tools, template replacements guide, main class and classpath determination). That is, outline all the specs. That way all the scripts will be in order and can be tested. Also, it is best to adopt a style guide for sh or bash scripts (e.g. this).

@muuki88
Copy link
Contributor

muuki88 commented Oct 31, 2019

Thanks a lot @archeuclid 😍

I wrote some minimal code to fix the issue.

This already looks like a full fledged solution 😎 I was wondering why you duplicate the settings for ash. In the end the user needs a single type of start script. The current AshScriptPlugin overrides / extends the bash script settings. I agree that the naming is not ideal. shell would probably be better.

Still duplicating adds a few issues.

  • we need to disable the BashScriptPlugin otherwise we have race conditions as the same script is generated by bash and ash
  • if we generate different scripts, we need to change entrypoints (e.g. the dockerEntrypoint)

Still need to add forwarder template, cygwin portability, and tests.

I wouldn't care if the cygwin portability is missing in the beginning. How runs cygwin in docker 😂 ? We can add this if someone really needs this.

I suggest to define some minimal specifications to all the scripts that are produced (e.g inherit or determine java launching tools, template replacements guide, main class and classpath determination). That is, outline all the specs. That way all the scripts will be in order and can be tested. Also, it is best to adopt a style guide for sh or bash scripts.

I really like this idea. I have to admit by bash knowledge is limited to the things I require (which is not much). We have very confusing summary of settings hidden in the archetype cheatsheet and Java Command Line Application / Start script options section and Start script customizations.

This would be a brilliant thing to merge this into a single documentation page. Or at least to have a single page that points to these different locations.

@archeuclid
Copy link

archeuclid commented Oct 31, 2019

I suggest that the start script feature should:

  1. Resolve java (from environment or whatever method, and all the version checking if necessary).
  2. Provide a default template and a list of replacements allowed in it (should be strictly outlined and checked somehow - someone in the future will inject something that will break things).
  3. Resolve main class.
  4. Resolve classpath.
  5. Resolve jvm options and jvm arguments (either environment or directly passed to the script).
  • There should be ordering priority - script args first, environment second, e.g.

No more, no less. That is no adding additional bundled jvm's, reading configs from other scripts etc. It is too much. Easier if all the responsibilities are outlined and are minimal. If there is a need for additional feature - create a new starter trait with all the things.

I refactored some more today. Will not PR until all the things are tested though.

@theon
Copy link

theon commented Mar 4, 2021

Hi, I was also receiving getting the line 79: addResidual: not found error, but upgrading to the 1.8.0 seems to have resolved the issue for me 👍

@muuki88 muuki88 closed this as completed Mar 5, 2021
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

4 participants