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

wip/ash #647

Merged
merged 1 commit into from
Aug 13, 2015
Merged

wip/ash #647

merged 1 commit into from
Aug 13, 2015

Conversation

gzoller
Copy link

@gzoller gzoller commented Aug 9, 2015

The current packager is bash-specific. If you want to use a BusyBox base Docker image then the current plugin has issues because BusyBox uses ash, not bash. Some of the more recent changes to the plugin are ash-incompatible (e.g. arrays in the script--ash doesn't have arrays). At first I thought I could get around this by overriding the template in my project but no dice. There are ash-incompatible issues in the template replacement for ${{template_declares}} -- these replacements are bash-specific, so I needed a new ash archetype. This pull request creates a simple ash-compatible launch script, without affecting the current bash capabilities. BusyBox is a pretty common lightweight base image so it'd be really great if this or a similar capability was pulled into master.

@muuki88 muuki88 added the universal Zip, tar.gz, tgz and bash issues label Aug 9, 2015
@muuki88
Copy link
Contributor

muuki88 commented Aug 9, 2015

Thanks a lot for your contribution :-) Code LGTM. There is some duplicated stuff, but I'll refactor this at some point.

Two things would be nice:

  • one test in sbt-test/universal that the ash script gets created
  • small hint in the docs what the difference between the two start scripts are ( in the Java application archetype )

@muuki88
Copy link
Contributor

muuki88 commented Aug 9, 2015

And could you squash everything to one commit?

@muuki88
Copy link
Contributor

muuki88 commented Aug 9, 2015

In #646 the ability to override the bash template is implemented. Do you think we could change this pr a little bit? I was thinking of shipping the ash template as a small archetype plugin, which sets the bashScriptTemplateLocation. We could remove the extra settings and the extra script.

@gzoller
Copy link
Author

gzoller commented Aug 9, 2015

That'd be cool but you'd need to fix the template_declares thing. A while ago a change was made to have a list of something in the code this expands to when substituted. This happens in the Java code now so only overriding templates won't fix that, unless I'm missing your idea. Other small things, like 'declare -r ...' isn't part of ash.

Let me know. I think what you're thinking of here would go beyond my limited understansing of the plugin. I'm good trying to make the changes you requested earlier but if there's a better way that's great too.

Greg

On Aug 9, 2015, at 4:42 PM, Nepomuk Seiler [email protected] wrote:

In #646 the ability to override the bash template is implemented. Do you think we could change this pr a little bit? I was thinking of shipping the ash template as a small archetype plugin, which sets the bashScriptTemplateLocation. We could remove the extra settings and the extra script.


Reply to this email directly or view it on GitHub.

@muuki88
Copy link
Contributor

muuki88 commented Aug 10, 2015

I see what you mean. I'll have a closer look when I'm home.

@muuki88
Copy link
Contributor

muuki88 commented Aug 10, 2015

So. I've come up with an idea. Introducing new settings/tasks and a new output bashscript is not a good idea as we could go on and on adding these. The solution is actually described in #635 . Overriding the makeBashScript task. This is pretty simple

  • Remove the new settings ashScript*
  • Create an AutoPlugin AshScriptPlugin in ...archetypes
  • Use your code makeAshScript <<= (ashScriptDefines, target in Universal, executableScriptName, sourceDirectory) map makeUniversalAshScript(ashTemplate), but override the makeBashScript task here.
  • Remove the new code from JavaApp

So you just move the logic into an own AutoPlugin and instead of creating a new task, you override the previous one (with the non-working declares and your template).

Your AutoPlugin should look something like

import sbt._

object AshScriptPlugin extends AutoPlugin {

   override def requires = JavaAppPackaging

   override def projectSettings = Seq(
     makeBashScript := // your code
   )
}

@gzoller
Copy link
Author

gzoller commented Aug 11, 2015

Interesting! Well I feel this is the right course too. Generating more and more scrips in the Java archetype isn't a scalable way forward. My challenge will be that I've never done a sbt plugin and there's a lot here that's a bit beyond my Scala Kung Fu, but I'm willing to give it a try. Let me play a few days and see how far I can get from your description.

Sent from my iPhone

On Aug 10, 2015, at 4:05 PM, Nepomuk Seiler [email protected] wrote:

So. I've come up with an idea. Introducing new settings/tasks and a new output bashscript is not a good idea as we could go on and on adding these. The solution is actually described in #635 . Overriding the makeBashScript task. This is pretty simple

Remove the new settings ashScript*
Create an AutoPlugin AshScriptPlugin in ...archetypes
Use your code makeAshScript <<= (ashScriptDefines, target in Universal, executableScriptName, sourceDirectory) map makeUniversalAshScript(ashTemplate), but override the makeBashScript task here.
Remove the new code from JavaApp
So you just move the logic into an own AutoPlugin and instead of creating a new task, you override the previous one (with the non-working declares and your template).

Your AutoPlugin should look something like

import sbt._

object AshScriptPlugin extends AutoPlugin {

override def requires = JavaAppPackaging

override def projectSettings = Seq(
makeBashScript := // your code
)
}

Reply to this email directly or view it on GitHub.

@muuki88
Copy link
Contributor

muuki88 commented Aug 11, 2015

If you need any help just ping me here. You have already written most of the code and with my small template above this should just be a matter of copy and paste :)

@gzoller
Copy link
Author

gzoller commented Aug 11, 2015

I think I'm close. I have AshScriptPlugin written and it appears to extend JavaAppPackaging properly. It generates ash-compatible replacement code. Since it leverages all the JavaAppPackaging infrastructure (including bash-template) you kinda have to override bash-template to get a fully ash-compatible result, but this seems to work well. As you thought earlier, this is a better way to do this leveraging existing infrastructure.

Last bit is I need to delve into the mysteries of the test code to create a test for it. (How do you run these tests outside your Travis build? Simple 'sbt test' doesn't run the tests in sbt-test, so I'm guessing there's something more to it.) After that I'll re-squash all this stuff and lift it back up for your review.

@muuki88
Copy link
Contributor

muuki88 commented Aug 11, 2015

Awesome! We have small Developer Guide, which describes how to run the tests, gen the docs, etc.

@gzoller
Copy link
Author

gzoller commented Aug 12, 2015

All pushed into the PR. Added test code on-par with the bash stuff under sbt-test. See what you think. Hopefully in line with what we discussed, which will be a better solution than my first attempt.

@muuki88
Copy link
Contributor

muuki88 commented Aug 12, 2015

Great job! +1 for the simple-app test that runs the create bash script and checks the output. You even got rid of your ash-template. I think this is a very good starting point, because IF we make some changes to the bash script that won't work in the ash-shell, we can still create a new one.

I'm waiting for travis (even though the service is acting pretty weird today) and the merge this right away. If you like you could add a small doc, either in Advanced Topics or the Docker Plugin. This is fine in another commit.

muuki88 added a commit that referenced this pull request Aug 13, 2015
@muuki88 muuki88 merged commit ea4f9cf into sbt:master Aug 13, 2015
@muuki88 muuki88 added docker and removed universal Zip, tar.gz, tgz and bash issues labels Aug 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants