Vojtech Szocs has posted comments on this change. Change subject: webadmin,userportal: Improve GWT build ......................................................................
Patch Set 6: (6 comments) http://gerrit.ovirt.org/#/c/36739/6/Makefile File Makefile: Line 34: EXTRA_BUILD_FLAGS= Line 35: BUILD_VALIDATION=1 Line 36: BUILD_ENV_VALIDATION=1 Line 37: BUILD_JAVA_OPTS_MAVEN=-XX:MaxPermSize=512M Line 38: BUILD_JAVA_OPTS_GWT=-Xms1024M -Xmx8192M -XX:PermSize=512M -XX:MaxPermSize=1024M > what happened to common aka BUILD_JAVA_OPTS_COMMON you wanted? > what happened to common aka BUILD_JAVA_OPTS_COMMON you wanted? It is not necessary. When we run Maven, BUILD_JAVA_OPTS_MAVEN defines extra Maven JVM options. When we run GWT compiler, its JVM inherits all "parent" (Maven) JVM options. Therefore there's no need for common JVM options. BUILD_JAVA_OPTS_GWT overrides any JVM options of "parent" (Maven) JVM. > don't you want to control this via environment? if you do replace the = with > ?= Sorry I am confused, can you explain what following does? ABC?=test Thanks. Line 39: DEV_REBUILD=1 Line 40: DEV_BUILD_GWT_DRAFT=0 Line 41: DEV_EXTRA_BUILD_FLAGS= Line 42: DEV_EXTRA_BUILD_FLAGS_GWT_DEFAULTS= Line 117: ifneq ($(BUILD_DEV),0) Line 118: BUILD_FLAGS:=$(BUILD_FLAGS) $(DEV_BUILD_FLAGS) Line 119: endif Line 120: BUILD_FLAGS:=$(BUILD_FLAGS) $(EXTRA_BUILD_FLAGS) Line 121: BUILD_FLAGS:=$(BUILD_FLAGS) -D gwt.jvmArgs="$(BUILD_JAVA_OPTS_GWT)" > why have you removed the EXTRA_BUILD_FLAGS? Above change just adds Maven gwt.jvmArgs property override to BUILD_FLAGS. Sorry, I don't understand your question, I see nothing removed, please elaborate. Line 122: Line 123: PYTHON_SYS_DIR:=$(shell $(PYTHON) -c "from distutils.sysconfig import get_python_lib as f;print(f())") Line 124: OUTPUT_RPMBUILD=$(shell pwd -P)/tmp.rpmbuild Line 125: OUTPUT_DIR=output Line 229: chmod a+x packaging/bin/ovirt-engine-log-setup-event.sh Line 230: Line 231: # support force run of maven Line 232: maven: Line 233: export MAVEN_OPTS="${MAVEN_OPTS} ${BUILD_JAVA_OPTS_MAVEN}" > please rework this into: OK, I'll make adjustments as suggested. > we do not need to export this. But we did it before, I didn't know we don't want to do this now. > also notice the () Thanks for noticing! I was unsure about this one. Line 234: $(MVN) \ Line 235: $(BUILD_FLAGS) \ Line 236: $(BUILD_TARGET) Line 237: touch "$(BUILD_FILE)" Line 498: ln -sf "$(DATA_DIR)/conf/osinfo-defaults.properties" "$(DESTDIR)$(PKG_SYSCONF_DIR)/osinfo.conf.d/00-defaults.properties" Line 499: Line 500: gwt-debug: Line 501: [ -n "$(DEBUG_MODULE)" ] || ( echo "Please specify DEBUG_MODULE" && false ) Line 502: $(MVN) -pl frontend/webadmin/modules/$(DEBUG_MODULE) \ > please keep the quotes > please keep the quotes OK. > not sure I see change in this hank that related to the flags, better to have > this in own patch, although it is not actually an improvement as it has the > same effect, no? :) Reference: http://books.sonatype.com/mvnref-book/reference/_using_advanced_reactor_options.html Maven -pl (or --projects) option allows specifying which projects we want to build, instead of default behavior (build everything in current context). Practical effect is the same, I removed "cd" command in favor of Maven -pl option. I've also verified this on my local development environment. Line 503: $(DEV_EXTRA_BUILD_FLAGS_GWT_DEFAULTS) \ Line 504: $(DEV_EXTRA_BUILD_FLAGS) \ Line 505: -Dgwt.noserver=true \ Line 506: -Pgwtdev,gwt-admin,gwt-user \ http://gerrit.ovirt.org/#/c/36739/6/frontend/webadmin/modules/pom.xml File frontend/webadmin/modules/pom.xml: Line 49: -Dgwt.jjs.permutationWorkerFactory=com.google.gwt.dev.ThreadedPermutationWorkerFactory \ Line 50: -Dgwt.jjs.maxThreads=4 \ Line 51: -Djava.io.tmpdir="${project.build.directory}/tmp" \ Line 52: -Djava.util.prefs.systemRoot="${project.build.directory}/tmp" \ Line 53: -Djava.util.prefs.userRoot="${project.build.directory}/tmp" \ > why have you removed the .java for properties root? Because it turns out that ".java" dir is created anyway, so before it was: target/tmp/.java/.java/.userPrefs and now it is: target/tmp/.java/.userPrefs I can revert this if you want. Line 54: ${gwt.jvmArgs} Line 55: </gwt-plugin.extraJvmArgs> Line 56: <!-- Custom JVM arguments for GWT compiler and Dev Mode, empty by default --> Line 57: <gwt.jvmArgs/> Line 53: -Djava.util.prefs.userRoot="${project.build.directory}/tmp" \ Line 54: ${gwt.jvmArgs} Line 55: </gwt-plugin.extraJvmArgs> Line 56: <!-- Custom JVM arguments for GWT compiler and Dev Mode, empty by default --> Line 57: <gwt.jvmArgs/> > I do expect sane default for jvmArgs for people which do not run maven via OK, so in Makefile I'll need to add check if BUILD_JAVA_OPTS_GWT is not empty - if so, override value of Maven gwt.jvmArgs property. Do you agree with above behavior? If not, please elaborate. Line 58: <!-- Control target browsers for GWT compilation, maps to 'user.agent' deferred binding property in *.gwt.xml --> Line 59: <!-- By default, compile for Firefox browser only, use 'all-user-agents' profile to compile for all browsers --> Line 60: <gwt.userAgent>gecko1_8</gwt.userAgent> Line 61: <!-- Control target locales for GWT compilation, maps to 'locale' deferred binding property in *.gwt.xml --> -- To view, visit http://gerrit.ovirt.org/36739 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaeb92d69f2ba38746559df3e44f34a61fd880908 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Barak Korren <[email protected]> Gerrit-Reviewer: David Caro <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Eyal Edri <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
