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

Implemented support for file permissions in webjars #3

Merged
merged 1 commit into from Jun 17, 2015

Conversation

jroper
Copy link
Member

@jroper jroper commented Jun 17, 2015

Ideally, we'd use file permissions from the jar files. The problem is, there is no standard for storing file permissions in zip files - Linux and other unices use ZipInfo which has its own proprietary extensions, but these are not implemented by, for example, the JDK, so that would mean any webjar that wanted to carry permissions would need to use a custom zip implementation that did support it both to extract and then package its jar (meaning a very custom maven setup that overrides mavens default package mechanism), and then in webjars itelf we'd need to have a custom zip implementation.

Rather than fighting this uphill battle, and out of band mechanism, in the form of a .webjars.permissions file, is defined. This is a properties file mapping webjars paths to permissions.

Implementing this was not quite straight forward, there were several bugs in the webjars extractor, including:

  • Extracting a single webjar from a filesystem classpath didn't work
  • When extracting webjars, the first file encountered, if it wasn't a node module and non node module was requested, was ignored. This usually wouldn't cause a problem because the first file encountered usually was not part of the webjar.

The jar file web jar extraction didn't really have any knowledge of what webjars it extracted, it assumed each jar file only contained one web jar (this doesn't seem like a good assumption to make, especially when fat jars are being used), and it meant hacks had to be done to determine the moduleId, and would have required more hacks to do the permissions.

So I rewrote the jar file webjar extraction to first discover and enumerate one or more webjars from each jar file, then moduleId detection and permissions could be easily applied per webjar.

@jamesward
Copy link
Member

I feel like I've spent half my life fighting with executable permissions on files in zips. So I'm glad that you were able to taste that slice of life.

jroper added a commit to jroper/webjars-npm that referenced this pull request Jun 17, 2015
This also adds file permissions as per:

webjars/webjars-locator-core#3

In order to fix:

sbt/sbt-js-engine#20
@jamesward
Copy link
Member

I could never figure out how to set the exec bit on files in zips using the standard Java zip libs. In the end I've resorted to using the system zip (which is what sbt-native-packager does).

One challenge with your approach is that somehow the Bower and NPM WebJars are going to need to auto-generate the .webjars.permissions file. Which is possible and possibly easier than generating a zip with the correct permissions. The Classic WebJars can just have manually created .webjars.permissions files which is definitely easier than figuring out how to get Maven to use the native zip.

One thing I didn't see in this PR was a test covering cases where the .webjars.permissions file and the contents of the WebJar do not align (missing files in either direction).

Before we merge this I'd like to verify that it is in fact hard to get Maven to use the native zip / preserve exec bits. If it turns out that is easy, then I think I'd prefer to go that route.

@jroper
Copy link
Member Author

jroper commented Jun 17, 2015

Ok, I just checked, it looks like maven does, by default, preserve permissions in its jar files when it packages. I've just updated webjars/npm#2, and verified that the files are executable. I'll update this PR to ensure that permissions are maintained when extracted.

@jroper
Copy link
Member Author

jroper commented Jun 17, 2015

I've updated the PR to simply preserve permissions coming from the webjar.

Note, this required adding a dependency on commons-compress, to get the support for reading the permissions, and upgrading to Java 7, since the APIs for setting file permissions were added in Java 7.

This ensures that permissions are maintained when webjars are extracted.
It uses commons-compress, which supports ZipInfo extensions, to read
permissions.  It requires Java 7, since Java 7 is required to work with
POSIX file permissions.

Implementing this was not quite straight forward, there were several
bugs in the webjars extractor, including:

* Extracting a single webjar from a filesystem classpath didn't work
* When extracting webjars, the first file encountered, if it wasn't a
  node module and non node module was requested, was ignored. This
  usually wouldn't cause a problem because the first file encountered
  usually was not part of the webjar.

The jar file web jar extraction didn't really have any knowledge of what
webjars it extracted, it assumed each jar file only contained one web
jar (this doesn't seem like a good assumption to make, especially when
fat jars are being used), and it meant hacks had to be done to determine
the moduleId, and would have required more hacks to do the permissions.

So I rewrote the jar file webjar extraction to first discover and
enumerate one or more webjars from each jar file, then moduleId
detection and permissions could be easily applied per webjar.
@jamesward
Copy link
Member

I'm not seeing the exec bit on the bins in the the latest npm WebJar:

~/projects/webjars/npm/target $ ls -al META-INF/resources/webjars/npm/2.11.2/bin/
total 32
drwxr-xr-x  3 j.ward  454177323   238 Jun  4 18:30 .
drwxr-xr-x  5 j.ward  454177323   204 Jun 17 06:28 ..
drwxr-xr-x  2 j.ward  454177323   136 Jun  4 18:30 node-gyp-bin
-rw-r--r--  1 j.ward  454177323   340 Jun  4 18:30 npm
-rw-r--r--  1 j.ward  454177323  1885 Jun  4 18:30 npm-cli.js
-rw-r--r--  1 j.ward  454177323   209 Jun  4 18:30 npm.cmd
-rw-r--r--  1 j.ward  454177323   504 Jun  4 18:30 read-package-json.js
~/projects/webjars/npm/target $ ls -al META-INF/resources/webjars/npm/2.11.2/bin/node-gyp-bin/node-gyp
-rw-r--r--  1 j.ward  454177323  172 Jun  4 18:30 META-INF/resources/webjars/npm/2.11.2/bin/node-gyp-bin/node-gyp

Are you seeing something different?

@jroper
Copy link
Member Author

jroper commented Jun 17, 2015

What version of maven are you using? I'm using 3.3.1.

@jroper
Copy link
Member Author

jroper commented Jun 17, 2015

And... did you do a clean?

@jamesward
Copy link
Member

Ah... I extracted with jar -xvf which doesn't support the exec bits. I just tried again with unzip and it worked!

jamesward added a commit that referenced this pull request Jun 17, 2015
Implemented support for file permissions in webjars
@jamesward jamesward merged commit a7207d9 into webjars:master Jun 17, 2015
@jamesward
Copy link
Member

Version 0.24 has been pushed to Maven Central. Thanks @jroper!

@benmccann
Copy link

Very cool! Thanks guys!

@jamesward @jroper I thought jars normally showed up in Maven Central within an hour? I'm still not seeing 0.24 available...

@jamesward
Copy link
Member

@benmccann Looks like it is there now: https://repo1.maven.org/maven2/org/webjars/webjars-locator-core/0.24/

But the http://search.maven.org index lags quite a bit.

@benmccann
Copy link

Oh, I see. It's because I was checking for webjars-locator instead of webjars-locator-core. Looks like we'll have to use webjars-locator to use the new webjars-locator-core. I'll send you a PR for that

@jamesward
Copy link
Member

We split out the RequireJS stuff and for backwards compat we had to move the core stuff to webjars-locator-core. So you can probably just switch your dependency from webjars-locator to webjars-locator-core.

@benmccann
Copy link

Sent the PR here for upgrading webjars-locator: webjars/webjars-locator#82

Play depends on webjars-locator: https://github.com/playframework/playframework/blob/master/framework/project/plugins.sbt

I kind of wanted to get this fix into the 2.4 branch and it might be too big a change to switch it from webjars-locator to webjars-locator-core unless jroper suggests otherwise

@jamesward
Copy link
Member

Since Play doesn't use the RequireJS parts it would actually be nice to switch to webjars-locator-core since webjars-locator brings in a ton of transitive deps. So lets see what James says before we make a decision on that PR.

@jroper
Copy link
Member Author

jroper commented Jun 18, 2015

For Play 2.4, it would be better to have a new webjars-locator. The problem is, if some dependency depends on an old, pre split webjars-locator, and we depend on webjars-locator-core, then both will end up on the classpath, which won't be good.

@jroper jroper deleted the permissions branch June 18, 2015 01:27
@jamesward
Copy link
Member

That is true. I'll work with @benmccann to get a new version of webjars-locator out.

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.

None yet

3 participants