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

allow uri-like pathes for app.root and gem.path #191

Closed
wants to merge 2 commits into from

Conversation

mkristian
Copy link
Member

when the app.root points to uri:classlaoder:// the rack-application can be packed one-to-one into WEB-INF/classes.
similar for setting gem.path to uri:classloader:// allows to embed the gems in WEB-INF/classes.

setting the gem.path is only needed for jruby-1.7.x and app.root only works with the 1.7.19+. all this allows a simplified packaging: https://github.com/mkristian/jruby-pack#packing-jarwar-files-of-ruby-application
and bypasses all the need of real-path.

@kares
Copy link
Member

kares commented Jan 5, 2015

@mkristian interesting thanks, while I appreciate the new direction this is getting super-ugly uri: special handling now leaking on 3 places ... will give it some more thoughts later.

@mkristian
Copy link
Member Author

a File.directory?(path) as check will achieve the same thing, i.e.
anything ruby can handle already does not need the extra realPath
resolution.

@mkristian
Copy link
Member Author

@kares I did reduce the patch to something generic ;)

@kares
Copy link
Member

kares commented Jan 6, 2015

looks better - thanks! but I'm not sure if it will pass the tests (will get 1.1-stable green just bare with me), the real_path code is a bit more responsible than it should be - I think it is expected to return nil (as getRealPath does) for resources that do not exists, otherwise things might not work as expected (e.g. Rails 2/3/4 detection).

what I'm thinking as an alternative (please let me know what you think) ... is introducing another layout since when all is under WEB-INF/classes it needs a bit different packaging than Warbler does right? thus configuring a layout sounds appropriate. let me know if I did miss/misunderstood something ...

@mkristian
Copy link
Member Author

yes, indeed a new layout will be the better way. even with the current
"patch" it needs quite some configuration in the web.xml to fit the
"alternative" packaging. so a new layout could be much better way to go.
thanx for the input - much appreciated.

@mkristian
Copy link
Member Author

@kares this a new layout - so there are no side effects for existing apps.

any idea how to get rid off:

 <context-param>
    <param-name>rackup</param-name>
    <param-value>eval File.read( 'uri:classloader://config.ru' )</param-value>
 </context-param>

when running jetty or tomcat without packing a war then there is no WEB-INF/classes - the classes directory is somewhere else and jruby-rack does not find the config.ru !?

@kares
Copy link
Member

kares commented Jan 7, 2015

looks good to me (assuming it works with the class-loader) that WebInf in the name evokes smt "special" about the layout ... have you considered something like ClassPathLayout (I know naming is the hard) ?

that eval is a uglish - probably we do not use the whole cp to locate the config.ru script while booting (it sure happens before the layout is set and used) ... might look into that in detail later if needed.

…under WEB-INF/classes

when the app.root points to uri:classloader:// the rack-application can be packed one-to-one into WEB-INF/classes.
similar for setting gem.path to uri:classloader:// allows to embed the gems in WEB-INF/classes.
@mkristian
Copy link
Member Author

so I did change the classname ;)

and added the search on context-classloader as last place to search for config.ru

@mkristian
Copy link
Member Author

@kares so from my side: I am excited. well, if this goes into 1.1-stable then I would add a small IT on jruby itself for it - it makes it easy to bundle a rack application with maven.

@kares
Copy link
Member

kares commented Jan 9, 2015

@mkristian looks good to go, I'll sure get this into 1.1.18 (will do some testing myself) ... I'll just need to figure out something around the ErrorApp - should be released next week (unless smt comes up). thanks!

@kares
Copy link
Member

kares commented Jan 11, 2015

on 1.1-stable with a cherry on top, thanks!

@kares kares closed this Jan 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants