Community
Participate
Working Groups
Created attachment 227710 [details] Patch to remove the task from projects under the aggregator The antrunner "replace" tasks should be eliminated by filtering the about.mappings files.
Should bugs/attachments like this one go against "Platform Releng", instead of "CBI Prototype"? I think that was the general advise/trend that started a few months ago? (Or, is there still a "prototype", in LTS, that I'm not aware of?) Also, for patches like this be useful to Eclipse Platform committers, they should be prepared in smaller pieces, ideally per-project, or, at least, "per-repo", since different committers will have to "apply" the patches to their specific code -- the specific pieces they have commit rights to. Changes to the aggregator itself can go to Platform, Releng, But jdt, eclipse.ui they need to go to those specific components. Also, I've not looked closely at these, but suspect most Platform committers want their patch in a form that can be applied by Eclipse (using EGit) ... on the surface these look like command line git diff patches which as far as I know can't be used directly by Eclipse/Egit. Paul, Thanh, please correct me if I'm wrong. But, thanks for the patches! You help will be appreciated.
(In reply to comment #1) > Should bugs/attachments like this one go against "Platform Releng", instead > of "CBI Prototype"? I think that was the general advise/trend that started a > few months ago? (Or, is there still a "prototype", in LTS, that I'm not > aware of?) Yes, these bugs should be moved to "Platform Releng" since the patches are specific to the Platform project.
(In reply to comment #1) > Also, for patches like this be useful to Eclipse Platform committers, they > should be prepared in smaller pieces, ideally per-project, or, at least, > "per-repo", Yes, the patches should be per-repo. > Also, I've not looked closely at these, but suspect most Platform committers > want their patch in a form that can be applied by Eclipse (using EGit) ... > on the surface these look like command line git diff patches which as far as > I know can't be used directly by Eclipse/Egit. Paul, Thanh, please correct > me if I'm wrong. I'm able to apply most patches that start with: diff --git a/.... I think EGit has adapted so it tries to strip off the first 1 or 2 segments before it tries to apply it. PW
We're still waiting for per-repo patches that can be applied. PW
Created attachment 229263 [details] Patch broken into per-repo patches Per-repo patches attached as archive.
Created attachment 229427 [details] eclipse.platform.patch
Created attachment 229428 [details] eclipse.platform.releng.patch
Thanh, and I only say Thanh because I think Dave Hughes has left already ... if not ... then Dave, :) Can you help sort out the regression this is causing. (see bug 406867) On the surface, it seems that those that applied the patch are now "broken", but those that still use 'antrun' are working ok. Bug query for related: https://bugs.eclipse.org/bugs/buglist.cgi?list_id=5320132&short_desc=%20[CBI]%20Remove%20antrunner%20replace%20tasks&classification=Eclipse&query_format=advanced&short_desc_type=allwordssubstr Is something wrong with the patch? Is there a simple fix? Or should those that apply it just un-apply it for now? (We'd like to have this working for M7, which only gives us a day). Did I forget to do something? I am 99.99% sure I do activate the profile for our production runs ... DEBUG: MARGS: -DbuildId=I20130430-0031 -X -Peclipse-sign -Pupdate-branding-plugins -Pbree-libs
A quick look seems like the patch is ok and I don't see anything missing from your command... I tried building just org.eclipse.cvs and found the about.mappings it produces does not replace any of the files so I'll take a closer look to see if I can figure out what's going on here.
Ok so looks like the resources plugin works in that it does copy the about.mappings and replaces the contents with the build property. But the problem seems to be that the build doesn't pick up this about.mappings file. Instead it includes the unmodified version. $ cat target/classes/about.mappings # about.mappings # contains fill-ins for about.properties # java.io.Properties file (ISO 8859-1 with "\" escapes) # This file does not need to be translated. 0=I20130430-1634 This makes sense though since build.properties doesn't say to pickup the version in target/classes/about.mappings I guess this is a problem with how "Maven" world handles things, it copies build output to the target folder. I guess the real question here is how do we get Maven / Tycho to pickup this alternative version?
Created attachment 230327 [details] workaround-org.eclipse.cvs Potential workaround... We move about.mappings file into a directory such as "build-resources" (name doesn't matter) Then use maven-resources-plugin's copy-resources goal to copy the about.mappings file out of the "build-resources" directory and into the root directory of the project. As a consequence the maven-resources-plugin will also filter and replace the variable ${buildId}. This will at least make it work similar to how we got the correct about.mappings file via the antrunner plugin with the bonus that git will no longer mark about.mappings as a modified file.
(In reply to comment #11) > Created attachment 230327 [details] > workaround-org.eclipse.cvs > > Potential workaround... > I'll prepare some full patches based on the workaround to test in case that's what we decide to go with. Unless a better solution comes up.
(In reply to comment #12) > (In reply to comment #11) > > Created attachment 230327 [details] > > workaround-org.eclipse.cvs > > > > Potential workaround... > > > > I'll prepare some full patches based on the workaround to test in case > that's what we decide to go with. Unless a better solution comes up. You patch appears to be against the original "antrun" code. Might I suggest you write a patch for JDT and PDE (only) since they are the ones with the current problem, since they've already applied the previous patch. That way, we could make sure it works, that they are willing to make that change, etc., and then after M7 work on replacement patches for the antrun case.
(In reply to comment #13) > You patch appears to be against the original "antrun" code. Yes, I was working out of master and I guess the latest code did not include the new patches yet. > Might I suggest you write a patch for JDT and PDE (only) Sounds good I'll do that. One thing I just thought of that I'm not sure if it'll cause issues is if about.mappings is moved to a folder like "build-resources" and the profile is not enabled, that means when someone runs a build without the profile they won't have the about.mapping file since it is not copied out of the directory. Would this file not existing cause issues with Eclipse in the final product? Alternatively what if we removed the profile and just have it always build with the branding plugins updated and defaulted ${buildId} to something like "Unsupported-${timestamp}" which would clearly indicate that a custom build was built?
(In reply to comment #14) > (In reply to comment #13) > > You patch appears to be against the original "antrun" code. > > Yes, I was working out of master and I guess the latest code did not include > the new patches yet. > > > Might I suggest you write a patch for JDT and PDE (only) > > Sounds good I'll do that. > > One thing I just thought of that I'm not sure if it'll cause issues is if > about.mappings is moved to a folder like "build-resources" and the profile > is not enabled, that means when someone runs a build without the profile > they won't have the about.mapping file since it is not copied out of the > directory. Would this file not existing cause issues with Eclipse in the > final product? > > Alternatively what if we removed the profile and just have it always build > with the branding plugins updated and defaulted ${buildId} to something like > "Unsupported-${timestamp}" which would clearly indicate that a custom build > was built? I like that idea, though might recommend "Custom-${timestamp}". [The words 'support' and 'unsupported' with open source are hard to interpret.] But, at same time, I don't think it'd hurt if "about.mappings" wasn't there ... that part of about.box would be blank, I'm assuming.
(In reply to comment #15) > I like that idea, though might recommend "Custom-${timestamp}". [The words > 'support' and 'unsupported' with open source are hard to interpret.] +1 I think all we'd have to change in the parent pom if we went this route is this line: <buildType>I</buildType> to <buildType>Custom-</buildType> Since buildId is already set to be <buildId>${buildType}${buildTimestamp}</buildId> > But, at same time, I don't think it'd hurt if "about.mappings" wasn't there > ... that part of about.box would be blank, I'm assuming. I guess we can find out when the workaround patches are accepted in Bug 406963 and Bug 406964
(In reply to comment #16) > (In reply to comment #15) > <buildType>I</buildType> to <buildType>Custom-</buildType> > > Since buildId is already set to be > <buildId>${buildType}${buildTimestamp}</buildId> > To be honest, I'd prefer the default "buildType" to be "N" and handle this "branding build id" with a separate field. Two reasons: it _probably_ would not effect the "pure" part of Tycho/Maven build ... and we always set buildType in production builds ... but, there might be places we infer the "buildType" by taking the first character of buildId. (In other words, you'd be surprised at what bugs might surface :) Second reason, eventually, I'd like to make the default buildType "N", and then "trigger" a bunch of other stuff off of that (e.g. see bug 404843). And somehow it seems more natural (I guess years of habit) to think of those types of "default, unsigned, non-production" builds as "N" builds, rather than "Custom-" builds. (Plus, then people could still do "N" builds, or whatever, and still set the ${brandingBundleId} to something more meaningful ... like "DavidsBuild-" :)
Comment regarding the two workaround patches for JDT and PDE: Moving to file to /build-resources is a no go. I want all features to have the same shape. Can you explain why the build id is correctly replaced for e.g. CVS and RCP but not for JDT and PDE?
(In reply to comment #18) > Comment regarding the two workaround patches for JDT and PDE: > > Moving to file to /build-resources is a no go. I want all features to have > the same shape. > > Can you explain why the build id is correctly replaced for e.g. CVS and RCP > but not for JDT and PDE? CVS and RCP branding bundles still use the "ant run" method. http://git.eclipse.org/c/platform/eclipse.platform.releng.git/tree/bundles/org.eclipse.cvs/pom.xml#n42 http://git.eclipse.org/c/platform/eclipse.platform.releng.git/tree/bundles/org.eclipse.rcp/pom.xml#n42 I think that's the other "workaround" for M7 ... if not Kepler ... is to revert that part of the patch and just go back to "antrun".
(In reply to comment #19) > (In reply to comment #18) > > Comment regarding the two workaround patches for JDT and PDE: > > > > Moving to file to /build-resources is a no go. I want all features to have > > the same shape. > > > > Can you explain why the build id is correctly replaced for e.g. CVS and RCP > > but not for JDT and PDE? > > CVS and RCP branding bundles still use the "ant run" method. > That's right, the "ant run" method allows in place replacement of a file while with Maven's resources plugin you cannot modify a file in place so the source and destination of said file must be different.
(In reply to comment #19) > I think that's the other "workaround" for M7 ... if not Kepler ... is to > revert that part of the patch and just go back to "antrun". I quickly talked to Paul and we agreed that we will just revert PDE and JDT for Kepler.
(In reply to comment #21) > (In reply to comment #19) > > I think that's the other "workaround" for M7 ... if not Kepler ... is to > > revert that part of the patch and just go back to "antrun". > > I quickly talked to Paul and we agreed that we will just revert PDE and JDT > for Kepler. Done.
I think best to consider this bug a dup of bug 419503. While the motivation behind 419503 is different, it has same effect as desired in this bug and would make this bug no longer required to fix. *** This bug has been marked as a duplicate of bug 419503 ***
