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

maven artifact resource roots #28

Closed
wants to merge 3 commits into from

Conversation

patriot1burke
Copy link
Contributor

I've added the ability to define a maven artifact as a resource-root

Tries to find a maven jar artifact from the system property "local.repository.path" This property is a list of platform separated directory names. If not specified, then it looks in ${user.home}/.m2/repository by default. If it can't find it in local paths, then will try to download from a remote repository from the system property "remote.repository". This
is not set by default so downloading doesn't happen by default. It will download both the pom and jar and put it into the first directory listed in "local.repository.path" (or the default dir). This directory will be created if it doesn't exist.

Finally, there is a progress download message. If you do not want a message to console, then set the system property "maven.download.message" to "false"

@dmlloyd
Copy link
Contributor

dmlloyd commented Feb 25, 2013

This kind of change has big non-obvious implications. I'll have to think about it a bit.

@patriot1burke
Copy link
Contributor Author

No special loader was implemented, its just taking XML and converting it to a file path. Downloading does not happen by default. Works even for -jar mode as you're still referencing local disk.

And you're right, it has big non-obvious positive implications. Implications I'd love to elaborate on.

@dmlloyd
Copy link
Contributor

dmlloyd commented Feb 28, 2013

A few questions:

  1. How robust is this artifact download strategy really? Would we be better off using something like Aether or is that overkill? (Asking honestly, I don't know at all).
  2. This should work on Windows as well I assume?
  3. I think it would be better to introduce a different element for maven resource roots, e.g. "maven-resource group="" artifact="" version="" classifier=""". Okay not a question so much.
  4. Assuming all above are answered - if you do the XSD changes to the 1.2 schema then I'm amenable to the change. Also not a question :)

@patriot1burke
Copy link
Contributor Author

  1. What does JBoss Modules do when an unloaded module is referenced by two separate threads at the same time? If JBoss Modules ensures only one thread is able to load the module (read the XML), then I see no problems with downloading. Concurrency was my only concern.
  2. I develop on Windows nowadays.
  3. Ok
  4. Ok

The only other thing I have concerns about is the Download message. It outputs progress using the "\r" trick which I think is important. Works great on the console, but, this results in a shit load of log file messages. I have another switch that allows you to turn this off.

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 1, 2013

Yeah module loading uses a lock to prevent the same module from being loaded in two threads (which is opposite from the way the concurrent class loading works). However you could still screw things up by having two modules refer to the same Maven artifact, thus it might be a good idea to protect downloads anyway.

TBH I'm not keen on the progress bar thing for download unless the user opts into it; I'd feel differently if there was a way to probe System.out to make sure it's still pointing at FileDescriptor.out but I don't see any way to do that without using reflection to get the value of the FilterOutputStream.out field, which is ugly on top of ugly...

@patriot1burke
Copy link
Contributor Author

removed the progress bar, just have one "Downloading xxx" message. It didn't work when running within jboss-as-maven-plugin anyways. Implemented all your suggestions and updated the XSD.

[artifact name="group:artifact:version[:classifier]"/]

Fernando suggested having one string for the FQN of the artifact. Might be useful to have system properties defining the artifact name, IDK....

Also added a [native-artifact name=""/] element. This will look for "lib/" in the artifact's repo directory and add this full path to a NativeLibraryResourceLoader. If the "lib/" directory isn't there, it will unzip the artifact jar in the artifact's repo directory. Don't know a better way of doing this at the moment. It assumes the JAR contains binaries with the same structure as jbossweb-native.

BTW, I don't know if Aether should be used or not. Would suck to have an external dependency for JBoss-Modules

@patriot1burke
Copy link
Contributor Author

I see that this didn't make 1.2.0.Final. Disappointed...Please let me know if I need to maintain this fork indefinitely along with my JBoss AS fork so I can leverage this feature.

@dmlloyd
Copy link
Contributor

dmlloyd commented Apr 11, 2013

Yeah, I had to rush out 1.2.0 before I wanted to because of EAP deadlines. This is still on my list though.

@dmlloyd
Copy link
Contributor

dmlloyd commented Apr 30, 2013

The stuff you've done for "native artifacts" - is that an existing Maven convention?

@patriot1burke
Copy link
Contributor Author

Native artifacts follows the same pattern currently used in AS8 for dealing with native artifacts: the extract-native-jar ANT macro used in AS8's build/build.xml file.

FYI, I'm currently using this successfully to replace my automated resteasy tests that Jetty, with jboss-as-maven-plugin pointing to a forked/patched version of AS8 distro that leverages this feature. I've also extended all the appropriate AS8 build scripts to build a parallel distro. I'm just waiting for this to be merged and released...

I think MODULES-146 is comple\imetary to my maven feature. What I currently have works seemlessly with jboss-as-maven-plugin and maven itself. Works great in development too as you can continually rebuild your maven projects and not have to change your AS8 distro (as long as they are pointing to the appropriate artifact you're rebuilding).

@dmlloyd
Copy link
Contributor

dmlloyd commented May 1, 2013

Merged, with a bit of cleanup. Bumped schema rev also.

@dmlloyd dmlloyd closed this May 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants