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

Support for absolute paths in classpath #882

Merged
merged 6 commits into from
Sep 28, 2016
Merged

Conversation

hayssams
Copy link
Contributor

Do not prefix with $lib_dir when an absolute path is added to the
classpath

Do not prefix with $lib_dir when an absolute path is added to the
classpath
@lightbend-cla-validator

Hi @hayssams,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@muuki88 muuki88 added the universal Zip, tar.gz, tgz and bash issues label Sep 22, 2016
@muuki88
Copy link
Contributor

muuki88 commented Sep 22, 2016

Thanks for your pull requests. Can you explain the reasons and use cases?

In order to accept the pull request we would also need a few things

  • Documentation on how to use it
  • Tests

@hayssams
Copy link
Contributor Author

When building a package you sometimes need to add in the classpath, an absolute path in your package like a docker volume path or any external path.
In my case, I needed to use hive-site.xml present on the target platform and located in an absolute path mounted as a docker volume.

You are right documentation and tests are missing and I have to add them asap.

Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Test case
  • Documentation on how to use

@muuki88
Copy link
Contributor

muuki88 commented Sep 23, 2016

Still need to get used to the new github review tools ;)

If you need any help on writing tests and documentation you can read the Developer Guide or ask here.

@hayssams
Copy link
Contributor Author

@typesafehub-validator CLA already signed

@hayssams
Copy link
Contributor Author

hayssams commented Sep 23, 2016

Hi @muuki88
Just added the test case but can't figure out where the best place is to put the following doc :

To add an absolute path to the app classpath just set it in the scriptClasspath Key as follows :
For Linux: scriptClasspath += "/absolute/path"
For Windows : scriptClasspath += "c:\absolute\path"
A relative path will be prefixed by the app lib directory path

Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work. Only a few tiny things :)

@@ -11,9 +11,10 @@ object JavaAppBatScript {

def makeWindowsRelativeClasspathDefine(cp: Seq[String]): String = {
def cleanPath(path: String): String = path.replaceAll("/", "\\\\")
def isAbsolute(path: String): Boolean = path.length > "c:\\".length && Character.isLetter(path(0)) && path(1) == ':'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about the "c:\\".lengthdefinition. It looks like putting documentation into the source code.
If you have to check for a path longer than 4 characters do so explicitly and add a small comment for what reason. At first I thought you were checking if the path starts with `c:\`` ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok will do it in a few minutes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -0,0 +1,21 @@
enablePlugins(JavaAppPackaging)
Copy link
Contributor

@muuki88 muuki88 Sep 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test looks good. However can you split this into two tests?

The bash one goes into sbt-test/bash and ( I know this is confusing :( ) into sbt-test/windows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@muuki88 muuki88 merged commit 148243d into sbt:master Sep 28, 2016
@muuki88
Copy link
Contributor

muuki88 commented Sep 28, 2016

Awesome. Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
universal Zip, tar.gz, tgz and bash issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants