Skip to content

WAR load support #608

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

Closed
wants to merge 9 commits into from
Closed

Conversation

mgudemann
Copy link
Contributor

This adds support to load class files from web application archive (WAR) files.
requires #604

@mgudemann mgudemann force-pushed the war_load_support branch 3 times, most recently from a8b2c79 to 015a52f Compare March 8, 2017 09:38
@@ -0,0 +1 @@
https://tomcat.apache.org/tomcat-6.0-doc/appdev/sample/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I very much appreciate that comment - but you might even place it in the test.desc file (so as not to create a non-standard file): you can place comments after another --, see test.pl --help

@@ -77,6 +78,11 @@ void jar_filet::open(
assert(filename_length==filename_len);
std::string file_name(filename_char);

// remove WAR class prefix
const std::string war_class_prefix("WEB-INF/classes/");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would that also be the prefix on Windows?

@@ -48,8 +77,27 @@ void jar_filet::open(const std::string &filename)
mz_zip_reader_get_filename(&zip, i, filename_char, filename_length);
assert(filename_length==filename_len);
std::string file_name(filename_char);

// remove WAR class prefix
const std::string war_class_prefix("WEB-INF/classes/");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would that also be the prefix on 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.

apparently, yes, cf. https://en.wikipedia.org/wiki/WAR_(file_format)
what's currently missing though, is loading of jars listed in WEB-INF/lib

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Maybe worth adding that link as a comment in the source code.)

@mgudemann mgudemann force-pushed the war_load_support branch 2 times, most recently from d3b91ff to 4ae4273 Compare March 13, 2017 18:35
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

A nit pick about blank lines, but mainly my please-change-this request is about the url.txt file that I'd prefer to see go away. See comment.

@@ -9,9 +9,9 @@ Author: Daniel Kroening, [email protected]
#include <cstring>
#include <cassert>
#include <unordered_set>

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather have more blank lines that fewer - is there a reason (such as IDEs doing automatic work) why this line is gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re-added this line and one in front of documentation block for the function afterwards
no particular reason for having removed it

@mgudemann
Copy link
Contributor Author

please note that this PR is not yet complete, as loading of jars in the war is not yet supported

@tautschnig
Copy link
Collaborator

Should it be tagged do-not-merge?

@mgudemann
Copy link
Contributor Author

currently, yes, but I cannot do the tagging apparently

@tautschnig tautschnig dismissed their stale review March 14, 2017 10:02

This is in do-not-merge state.

@tautschnig tautschnig changed the base branch from master to develop August 22, 2017 12:32
@mgudemann
Copy link
Contributor Author

I brought this up to current develop but it still lacks support for .jar files in .war files. We probably could extract the packaged jars into temp files, load them and then delete them again? Or we can see how miniz supports extracting archives from memory. This will certainly require low-level API use.

@thomasspriggs
Copy link
Contributor

I am going to close this PR as it appears that work on it has been inactive for an extended period of time. This is not any judgement on how worthwhile this work is. This is part of an effort to reduce the number of open PRs which are not being actively worked on. If you still think it would be worthwhile to get this merged then the branch should be re-based on the latest version of develop and the PR re-opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants