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

Fix buildWAR target #85

Merged
merged 1 commit into from Apr 16, 2013
Merged

Fix buildWAR target #85

merged 1 commit into from Apr 16, 2013

Conversation

davido
Copy link
Contributor

@davido davido commented Apr 15, 2013

Fist of all thank you for integrating JGut 3.0.0-SNAPSHOT! I wonder why line 163 in RepositoriesPage.java is failing to load welcome.mkd? It is in place in gerrit-gitblit.jar in root:

jar -tf gitblit.jar | grep -i welcome.mkd
welcome.mkd

Instead of welcome message i see this (from Gerit GitBlit Plugin):

Failed to read default message from welcome.mkd!

I am going to check the vanilla war and was trying to build it and it failed:

BUILD FAILED
/home/davido/projects/gitblit_src/build.xml:272: org.moxie.MoxieException: Unable to resolve: com/gitblit/GitServlet.class

com/gitblit/GitServlet.class could not be located on the classpath.

at org.moxie.ant.MxGenJar.execute(MxGenJar.java:405)

So i adjusted the path. Not sure if GitServlet.java should be moved?

Thanks
David

@gitblit gitblit merged commit e329791 into gitblit-org:master Apr 16, 2013
@gitblit
Copy link
Collaborator

gitblit commented Apr 16, 2013

Oops. I moved the servlet while I was traveling and writing the gitdaemon support. I don't often buildWAR but eventually I would have caught it. Thanks for the patch!

I did move the markdown files from being web resources to being classpath resources. I'll take a look at it tomorrow... unless you find it first. :)

@davido
Copy link
Contributor Author

davido commented Apr 16, 2013

I did move the markdown files from being web resources to being classpath resources.

Ah, thank you for the hint. I checked the current implementation of gerrit-gitblit plugin.
They inhertied GerritGitBlit from GitBlit:

@singleton
public class GerritGitBlit extends GitBlit {

and have overridden the getResourceAsStream(String).
Clearly after moving it from web resource to classpath resource, things are different now.

@OverRide
public InputStream getResourceAsStream(String file)
throws ResourceStreamNotFoundException {
String resourceName = "/static/" + file;
InputStream is = getClass().getResourceAsStream(resourceName);
if (is == null) {
throw new ResourceStreamNotFoundException("Cannot access resource "
+ resourceName + " using class-loader " + getClass().getClassLoader());
}

return is;

}

What i didn't understand, is what exactly Apache Wicket method doing:
public static ContextRelativeResource getResource(String file) {
return new ContextRelativeResource(file);
}

I will look into it further, until you solve it first ;-)

Thanks
David

@gitblit
Copy link
Collaborator

gitblit commented Apr 16, 2013

This is now fixed. Bizarre. I remember fixing this when I moved the resources. I must have switched branches and reverted or something.

As for loading reference.properties, I think we no longer need changes. The original code was a workaround for loading a "static" web resource, not a classpath resource.

I assume that Gitblit-Gerrit must also subclass WicketUtils to control where context relative resources are loaded. Can you confirm? I think we can probably improve that.

@davido
Copy link
Contributor Author

davido commented Apr 16, 2013

Thank you for fixing it, sorry for not seeing it and fixing it myself ;-)

As for loading reference.properties, I think we no longer need changes.

Agree. i am not a friend of duplication of entire reference.properties for gerit-gitblit-plugin anyway. So currently i am trying to overwrite two properties that we actually need (and that are different from vanilla GitBlit version) and removed the whole code and reference.properties from plugin, here the updated patch:

https://gerrit-review.googlesource.com/#/c/44263/

What do you think?

I assume that Gitblit-Gerrit must also subclass WicketUtils to control where context relative resources are loaded.

I guess not, because gerrit-plugin machinery does some magic here. It was working in 1.2.0 and it is still working.

I've two questions (not sure if it is the best place to ask ;-)

  1. gitblit.war is not working ... i deployed it on my tomcat, and tried host:port/gitblit and got 404, any idea why?
  2. ant test target produces failure, but reports success, is it kind expected?

[...]
test:
[mx:javac] =========================================================
[mx:javac] mx:Javac (com.gitblit:gitblit:1.3.0-SNAPSHOT, test)
[mx:javac] ---------------------------------------------------------
[mx:javac] Cleaning /home/davido/projects/gitblit_src/build/test-classes
[mx:javac] Compiling 47 source files to /home/davido/projects/gitblit_src/build/test-classes
[mx:javac] Note: Some input files use or override a deprecated API.
[mx:javac] Note: Recompile with -Xlint:deprecation for details.
[mx:javac] Copying 1 file to /home/davido/projects/gitblit_src/build/test-classes
[mx:test] =========================================================
[mx:test] mx:Javac (com.gitblit:gitblit:1.3.0-SNAPSHOT, test)
[mx:test] ---------------------------------------------------------
[mx:test] Cleaning /home/davido/projects/gitblit_src/build/test-classes
[mx:javac] Compiling 47 source files to /home/davido/projects/gitblit_src/build/test-classes
[mx:test] Note: Some input files use or override a deprecated API.
[mx:test] Note: Recompile with -Xlint:deprecation for details.
[mx:javac] Copying 1 file to /home/davido/projects/gitblit_src/build/test-classes
[test] Running com.gitblit.tests.ActivityTest
[test] Tests run: 1, Failures: 0, Errors: 1, Time elapsed: 3.245 sec
[test] TEST com.gitblit.tests.ActivityTest FAILED
[test] Running com.gitblit.tests.ArrayUtilsTest
[test] Tests run: 3, Failures: 0, Errors: 0, Time elapsed: 0.008 sec
[test] Running com.gitblit.tests.Base64Test
[test] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.008 sec
[test] Running com.gitblit.tests.ByteFormatTest
[test] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.009 sec
[test] Running com.gitblit.tests.DiffUtilsTest
[test] Tests run: 8, Failures: 0, Errors: 7, Time elapsed: 0.083 sec
[test] TEST com.gitblit.tests.DiffUtilsTest FAILED
[...]

BUILD SUCCESSFUL
Total time: 2 minutes 1 second

Thanks
David

@gitblit
Copy link
Collaborator

gitblit commented Apr 16, 2013

reference.properties in Gitblit is not being used the same as reference.properties in Gitblit-Gerrit. Perhaps it should have been named default.properties. Gitblit-Gerrit is using it as the actual settings file, not the default/reference values. There are probably several settings that Gitblit-Gerrit will want to change from the default values. Gitblit uses defaults in code if the provided settings/properties class does not have a requested setting value.

  1. gitblit.war not working. 404. Hmm sounds like a path problem (is it really /gitblit?) or perhaps a failure to deploy the war. Is there a stacktrace from Tomcat?
  2. The unit tests are complex and can be finicky. I wouldn't worry about failures there at the moment. They could be related to switching to 3.0.0-SNAPSHOT, not sure. Another thing to add to the list. :)

@davido
Copy link
Contributor Author

davido commented Apr 17, 2013

Well, Gitblit-Gerrit's own com.googlesource.gerrit.plugins.gitblit.reference.properties was loaded and passed as InputStream to GitBlit.contextInitilaized() method and passed from there to loadSettingModels(), where GitBlit's own "/reference.properties" is loaded.

Opps, while writing this i realized, that holds true for current gitblit.jar (based on 1.3.0-SNAPSHOT), that was built with extended moxie -DresourceFolderPrefix=static. I checked the current production gitblit.jar that was built by GerritForge:

gerrit2@lvps46-163-74-91:~/gerrit_testsite/plugins$ jar -tf gitblit.jar | grep -i reference.properties
com/googlesource/gerrit/plugins/gitblit/reference.properties

So the question is still how we can keep the default.properties in GitBlit and only pass different settings to GitBlit from GitBlit-Gerrit?

GitBlit war & junit: i will take a look.

Thanks
David

@gitblit
Copy link
Collaborator

gitblit commented Apr 17, 2013

That looks like what the original Gitlbit-Gerrit code does. It loads Gerrit's gitblit settings file (reference.properties) as Gitblit's default settings. It then manually overrides a setting and passes that config to Gitblit. The last step is to pass the inputstream of Gerrit's properties file to Gitblit to load the settings models used for RPC setting management.

@davido
Copy link
Contributor Author

davido commented Apr 17, 2013

The last step is to pass the inputstream of Gerrit's properties file to Gitblit to load the settings models used for RPC setting management.

This is exactly what old code GitBlit.java did that is removed now and you also wasn't amused to integrate it again with objection (that i agree with) to not pass InputStream around.

So that i am asking what would be the best way to proceed here?

Thanks
David

@gitblit
Copy link
Collaborator

gitblit commented Apr 17, 2013

Right. I was explaining what current Gitblit-Gerrit does since there is a difference between the GitBlit.settings object and the GitBlit.settingsModel object (where reference.properties is used). Read the GitBlit.loadSettingModels method to understand how reference.properties is used.

I think you can restore the old code and eliminate the inputstream parameter to contextInitialized. Gitblit-Gerrit does not use Gitblit's RPC functions so it will not care about what is loaded into settingsModel. The RPC mechanism (disabled by default) is currently the only place where the settingsModel is used.

Mashing Gitblit & Gerrit together is a little like putting a square peg into a round hole. With enough force you can do it - but it wasn't designed for it.

@gitblit
Copy link
Collaborator

gitblit commented Apr 17, 2013

You do want to pay attention, though, for what should be the default values for Gitblit-Gerrit.

If you continue with your patch you will be using stock Gitblit GO settings, aside from the 2 you are manually setting. So either you keep a properties file for creating GitblitSettings() OR, in code, you override all relevant defaults. Some of the Gitblit GO settings may not make sense for Gerrit (RPC) or may adversely affect Gerrit. Don't ask me which ones, I don't run Gerrit.

@davido
Copy link
Contributor Author

davido commented Apr 18, 2013

Thank you for these suggestions! I am going to attend Gerrit Spring Hackathon in London (7-9 May) and to meet Luca there and discuss it with him, so let us summarize the options we have:

  1. Add new method protected String getResourcePrefix() or some such to GitBlit. Vanilla GitBlit implementation just return "/", while overriden method returns "", making it possible to load alternative reference.properties, located in different path. In this scenario we would still have two reference.properties in classpath, one from GitBlt and another from Gerrit-GitBlit.
  2. Make sure that GitBlit.settingsModel actually don't affect Gerrit-GitBlt in any way (may be rename it to rpcSettingsModel to make it even more clear what it is used for), let to load the vanilla reference.properties, like in my current patch, and may be extract the overloaded properties from be loaded from code to gerrit-gibtlit.properties, or some such. In this scenario we would have one vanilla reference.properties.
  3. patch build.xml to add another property or even task (a lá instammGerritGitBlitMaven) to strip vanilla reference.properties from gitblit.jar. In this case, Gerrit-GitBlt can jus put its own reference.properties in root of classpath, and current loading code getResourceAsStream("/refrerence.properties") would just work. In this scenario we would have one customized reference.properties.

In all these options we don't pass any InputStream(s) around ;-)

What do you think?

Thanks
David

@davido
Copy link
Contributor Author

davido commented Apr 18, 2013

Looking into Tomcat gitblit.war problem: it seems that i might be missing some RTFM? Could you point me to it?
;-)

tomcat log:

INFO: Deploying web application archive /opt/tomcat/apache-tomcat-7.0.39/webapps/gitblit.war
INFO WAR contextFolder is /opt/tomcat/apache-tomcat-7.0.39/webapps/gitblit
INFO Gitblit base folder = /opt/tomcat/apache-tomcat-7.0.39/webapps/gitblit/WEB-INF/data
INFO Git repositories folder = /opt/tomcat/apache-tomcat-7.0.39/webapps/gitblit/WEB-INF/data/git
INFO Gitblit settings = /opt/tomcat/apache-tomcat-7.0.39/webapps/gitblit/WEB-INF/data/gitblit.properties
INFO Identifying available repositories...
INFO 0 repositories identified with calculated folder sizes in 11 msecs
INFO JVM timezone is Europe/Berlin (CEST +0200)
INFO Gitblit timezone is Europe/Berlin (CEST +0200)
INFO Setting up user service GitblitUserService
INFO GUS delegating to ConfigUserService(/opt/tomcat/apache-tomcat-7.0.39/webapps/gitblit/WEB-INF/data/users.conf)
WARN Mail server is not properly configured. Mail services disabled.
INFO Lucene executor is scheduled to process indexed branches every 2 minutes.
WARN Federation passphrase is blank! This server can not be PULLED from.
Apr 18, 2013 10:48:01 AM org.apache.catalina.core.StandardContext startInternal
SEVERE: Error listenerStart
Apr 18, 2013 10:48:06 AM org.apache.catalina.util.SessionIdGenerator createSecureRandom
INFO: Creation of SecureRandom instance for session ID generation using [SHA1PRNG] took [5,012] milliseconds.
Apr 18, 2013 10:48:06 AM org.apache.catalina.core.StandardContext startInternal
SEVERE: Context [/gitblit] startup failed due to previous errors

Thanks
David

@gitblit
Copy link
Collaborator

gitblit commented Apr 18, 2013

Option 4. Gitblit-Gerrit forgets "reference.properties" exists and does nothing about loading it. (My preference). I do like the idea that you should have a "gitblit-gerrit.properties" file where you configure how Gitblit-Gerrit will work, rather than just stuffing values in code. The properties file has descriptions which would be useful for other Gitblit-Gerrit contributors.

It seems like the getResourcePrefix method is not needed either.

WAR startup. I just built WAR and deployed to a fresh, vanilla Tomcat 7.0.39. I didn't change any settings and it worked as expected. Are you already running a GitDaemon service on this machine?

@davido
Copy link
Contributor Author

davido commented Apr 18, 2013

Option 4. [...]

done: https://gerrit-review.googlesource.com/#/c/44263/

WAR startup. [...] Are you already running a GitDaemon service on this machine?
nope, i don't. Is that the problem?

Thanks
David

@davido
Copy link
Contributor Author

davido commented Apr 20, 2013

I found the problem with WAR deployment: i started Gerrit on that machine already ... with gerrit-gitblit-plugin enabled, in which case gitblit-gerrit started already GitDaemon under the port 9418. GtBlit-War in tomcat failed to start because that port was already in use ;-)

The same problem i had on another productive machine, where i tried to deploy just released Gerrit 2.6-rc1 by Shawn with my new and shiny gerrit-gitblit plugin, but somehow MessageFormat rendering failed:

Caused by: java.lang.IllegalArgumentException: unknown format type at
at java.text.MessageFormat.makeFormat(MessageFormat.java:1442)
at java.text.MessageFormat.applyPattern(MessageFormat.java:458)
at java.text.MessageFormat.(MessageFormat.java:350)
at java.text.MessageFormat.format(MessageFormat.java:811)
at com.gitblit.GitBlit.configureGitDaemon(GitBlit.java:3238)
at com.gitblit.GitBlit.configureContext(GitBlit.java:3131)
at com.googlesource.gerrit.plugins.gitblit.GerritWicketFilter.init(GerritWicketFilter.java:91)

The problem was again that port 9418 already in use. Not sure if it is a bug in that line?
logger.error(MessageFormat.format("Failed to start Git daemon on {0}:{1,number.0}", bindInterface, port), e);
Can you confirm?

Anyway, i disabled GitDaemon service in gitblit-gerrit, i think it doesn't make any sense in that context
Right?

https://gerrit-review.googlesource.com/#/c/44263/

Thanks
David

@gitblit
Copy link
Collaborator

gitblit commented Apr 22, 2013

There are probably several settings that do not make sense for Gitblit-Gerrit. That is one of them. I would replace your existing gitlbit-gerrit.properties file with the current gitblit.properties from master, review all settings, and tweak the ones that should be changed.

The exception you are reporting was fixed on master last week.

gitblit added a commit that referenced this pull request May 23, 2014
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

2 participants