Community
Participate
Working Groups
It's a follow-up to bug 345669, where we decided to commit only the GitCopyrightAdapter (copyright section updater for git) but not the unit test for it. It's about to happen here once David figures out how to include org.eclipse.jgit.junit dependency.
Created attachment 214788 [details] GitCopyrightAdapter - unit test A copy of attachment 212147 [details] from bug 345669.
In bug 345669 comment 22 David suggested to copy-paste part of the code from org.eclipse.jgit.junit required by the unit test. This way we could remove the problematic dependency. It's definitely worth a try, so David please hold off with looking for a fix for the dependency. I will redo the patch and try to remove it.
Created attachment 216568 [details] GitCopyrightAdapter - unit test Copied part of LocalDiskRepositoryTest to o.e.releng.tests so that the dep on o.e.jgit.junit can be dropped.
Created attachment 216569 [details] mylyn/context/zip
This patch looks good to me. A couple of suggestions. Since the Java pre-req is required to move up to 1.5, we should announce that on platform-releng-dev list, though can't imagine there would be any problems doing so (since so many others are moving "up"). For the copied file, I didn't see a note about where it originally came from. Normally, I like to see a small not about origin, just in case, in future, anyone needs to check original. Something like the following would do. /* * Copyright (C) 2009-2010, Google Inc. * Copyright (C) 2008, Robin Rosenberg <robin.rosenberg@dewire.com> * Copyright (C) 2007, Shawn O. Pearce <spearce@spearce.org> * and other copyright owners as documented in the project's IP log. * * This class was originally copied from * org.eclipse.jgit.junit.LocalDiskRepositoryTest * See https://bugs.eclipse.org/bugs/show_bug.cgi?id=378047 * * ...
(In reply to comment #5) > For the copied file, I didn't see a note about where it originally came > from. The class' javadoc says "An extract from org.eclipse.jgit.junit.LocalDiskRepositoryTestCase", but mentioning the origin in the copyright section sounds like a better idea indeed.
Created attachment 222446 [details] GitCopyrightAdapter - unit test (for Java 1.4) Added a note about origin of the class and removed the Java 1.5 req.
Looks good to me. Thanks.
Oh, I guess technically the version should be increased, say to 3.2.300.qualifier (from its current 3.2.200.qualifier) And ... my preference ... I think best if this just goes into 'master'. Not sure its worth touching maintenance branches ... but ... I could see an argument either way.
Arrgg, I forgot my workspace was still "rebuilding" after I applied your latest patch. Sorry. But think there's some issues. I'm seeing compile errors. I think all related to you moving back to 1.4. The following sorts of things require 1.5. The compile errors go away if I change to 1.5. I think that's fine. (Appears necessary?) import static org.junit.Assert.assertEquals; @Test
(In reply to comment #10) > I'm seeing compile errors. I think all > related to you moving back to 1.4. The following sorts of things require > 1.5. The compile errors go away if I change to 1.5. (...) > > import static org.junit.Assert.assertEquals; > > @Test Hmm, I don't see any static import in the latest patch. Are you sure you applied the right one?
(In reply to comment #11) > (In reply to comment #10) > > I'm seeing compile errors. I think all > > related to you moving back to 1.4. The following sorts of things require > > 1.5. The compile errors go away if I change to 1.5. (...) > > > > import static org.junit.Assert.assertEquals; > > > > @Test > > Hmm, I don't see any static import in the latest patch. Are you sure you > applied the right one? Yes, but I applied it wrong. I forgot a "reset hard" does not remove the new files (from previous patch) and when I applied new one I didn't look very closely but it did not overwrite the old ones ... so ... all looks well now that I've done it more carefully. Thanks again.
No worries, is a go then? Assuming I will apply what you suggested in comment 9. I guess I should bump the version in pom.xml as well, right?
(In reply to comment #13) > No worries, is a go then? Assuming I will apply what you suggested in > comment 9. I guess I should bump the version in pom.xml as well, right? Yes and yes.
Fixed with ac72882cbe03dc60c1c910000b675e227acbd3bb, thanks David for looking at this with me.
(In reply to comment #15) > Fixed with ac72882cbe03dc60c1c910000b675e227acbd3bb, thanks David for > looking at this with me. In the future please paste a complete link http://git.eclipse.org/c/platform/eclipse.platform.releng.git/commit/?id=ac72882cbe03dc60c1c910000b675e227acbd3bb so that one can directly easily look at the changes.
The test is not executed (see http://download.eclipse.org/eclipse/downloads/drops4/N20121017-2000/testResults.php). You probably missed to add it to the test suite that's launched via test.xml.
There is another (worse) problem we missed. jgit _is_ still required. This broke tonight's Nighty tests (not builds) since installing the tests failed with Eclipse Releng Tests 3.2.300.N20121018-2000 (org.eclipse.releng.tests 3.2.300.N20121018-2000) requires 'bundle org.eclipse.jgit 1.3.0' but it could not be found. I will revert ac72882cbe03dc60c1c910000b675e227acbd3bb until we get this sorted out. I removed the jgit bundle from manifest and there were several compile errors, so ... not sure if I misunderstood ... or ... just overlooked?
(In reply to comment #18) > There is another (worse) problem we missed. jgit _is_ still required. This > broke tonight's Nighty tests (not builds) since installing the tests failed > with There is another thing you need to be careful with: EGit must not be available/installed when running the other tests. Why? Because the latest stable version opens two warning dialogs. See bug 391377 for details. This would block some of our tests.
Gee, I thought the only dep we need to get rid of is org.eclipse.jgit.junit, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=345669#c35. So maybe it's better to mark the jgit dep as optional, just like we did in org.eclipse.releng.tools, see http://git.eclipse.org/c/platform/eclipse.platform.releng.git/commit/?id=8e55c0a9b0932fee65c21ed9ba1ff0d74a024d82
Probably would be best if marked optional. What would the test do then? I assume catch a "class not found" error and print a warning message to log "xyz test could not run since jgit/egit was not installed"? [But, I would not want it to "fail"]. Then perhaps another bug open to install jgit/egit during our production run tests [Thanks for the warning about "latest version", Dani ... but I think during build, we just take "released" versions to compile against, so I think fairly predictable]. And, yes, the other action (since we went back to 1.4 and removed @Test, I believe was the reason) is to figure out how to get the test to run during tests. It seems current ones run by an explicit task in test.xml. Honestly, I'm not used to doing things that way, but seems like others do it and and "call" several test suites from test.xml. I'm unsure of pros/cons, alternatives ... so advice/preferences/recommendations welcome.
(In reply to comment #21) > Then perhaps another bug open to install jgit/egit during our production run > tests [Thanks for the warning about "latest version", Dani ... but I think > during build, we just take "released" versions to compile against, so I > think fairly predictable]. I don't understand what you try to say here. The problem (opening dialogs) is in the latest released EGit version. So, yes, the hang will be predictable but not good for the tests.
(In reply to comment #22) > (In reply to comment #21) > > Then perhaps another bug open to install jgit/egit during our production run > > tests [Thanks for the warning about "latest version", Dani ... but I think > > during build, we just take "released" versions to compile against, so I > > think fairly predictable]. > > I don't understand what you try to say here. The problem (opening dialogs) > is in the latest released EGit version. So, yes, the hang will be > predictable but not good for the tests. Oh, when you said "latest stable version" I thought you meant latest milestone.
Not sure where we stand here. I'm not crazy about adding a new prereq (jGit/eGit) to all unit tests, just for this one "tools" case. Perhaps an alternative approach is to get to the point where we can build (and test) these tools by themselves? I'd feel fine about including the prereq in that scenario. Tomasz are you considering reworking the bundles/tests so they "fail gracefully" with pre-reqs not there? Such as just write a message to log, but not "fail" a unit test? Or, are you just waiting on me to add in as a pre-req? Thanks,
(In reply to comment #24) > Not sure where we stand here. I'm not crazy about adding a new prereq > (jGit/eGit) to all unit tests, just for this one "tools" case. +1.
(In reply to comment #24) > Tomasz are you considering reworking the bundles/tests so they "fail > gracefully" with pre-reqs not there? Such as just write a message to log, > but not "fail" a unit test? Yep, it's on my list, but it's not even in the top 10. I will try to find some time just after xmas break.
(In reply to comment #17) > You probably missed to add it to the test suite that's launched via test.xml. Does the script need any extra params to run? Run As > Ant Build... results in: BUILD FAILED D:\workspace\eclipse\jdt\eclipse.platform.releng\bundles\org.eclipse.releng.tests\test.xml:111: The following error occurred while executing this line: java.io.FileNotFoundException: D:\workspace\eclipse\jdt\eclipse.platform.releng\plugins\org.eclipse.test\library.xml
(In reply to comment #27) > (In reply to comment #17) > > You probably missed to add it to the test suite that's launched via test.xml. > > Does the script need any extra params to run? Run As > Ant Build... results > in: > > BUILD FAILED > D:\workspace\eclipse\jdt\eclipse.platform.releng\bundles\org.eclipse.releng. > tests\test.xml:111: The following error occurred while executing this line: > java.io.FileNotFoundException: > D:\workspace\eclipse\jdt\eclipse.platform.releng\plugins\org.eclipse. > test\library.xml I'm not sure how to run the "test.xml" file, from workbench. You'd have to specify, at least, where the "library.xml" file is, and its a little different in the workbench, than in a build. Such as, assuming you have org.eclipse.test loaded, you could try -Dlibrary-file=/home/davidw/git/eclipse.platform.releng/bundles/org.eclipse.test/library.xml But, I tried that, and it'd fail later, and I couldn't figure out an easy way forward. Normally, when I run tests from the workbench, I just select the "BuildTests" class and "run as Junit plugin test". (Even then many fail, since many of the "input files" its normally looking for, after a build, do not exist in workspace. There are some instructions for running the tests from a "standalone" install, at http://wiki.eclipse.org/Platform-releng-faq#How_do_I_run_the_JUnit_tests_used_in_the_build.3F which point to http://download.eclipse.org/eclipse/downloads/drops4/R-4.2.1-201209141800/automatedtesting.html But not sure they are up-to-date enough to be helpful? If you get the "BuildTests" run-as-junit-plugin-test method to work for your case (e.g. with/without EGit/JGit), I'd say that'd be close enough.
Created attachment 228319 [details] GitCopyrightAdapterTest v04 David, please let me know if this one is any better.
Created attachment 228320 [details] mylyn/context/zip
I've applied this patch and willing to give it a try. But, some questions, still. I noticed that you removed "org.eclipse.pde.tools.versioning" from the imports? I think we might not really need it as a manifest requirement, and might have been "cheating" by having it there. ... when in fact we (at least) need it in the test feature? I assume the idea is these would not run as part of automated tests, since egit/jgit not available, but the idea is you (or others) could easily run from the workspace. Right? I didn't work for me ... but, I don't trust my environment is set up very well for "running" things. Can you detail or confirm your procedure? What I tried was to select your AllRelengTests and said "run as junit test" and got some (vague) error that some service could not be obtained. Much thanks for your persistence and continued support.
FYI, I see now we already have org.eclipse.pde.tools.versioning in test feature, so no need to change there, and I could not find a reason it needed to be in manifest.mf ... even though we refer to it in the test.xml file ... I believe it can find any "application" that's installed. Thanks.
(In reply to comment #31) > I noticed that you removed "org.eclipse.pde.tools.versioning" from the > imports? This was not intentional. I removed it locally because it caused me some compilation errors. Do not apply that part. Or revert it if you already have. > I assume the idea is these would not run as part of automated tests, since > egit/jgit not available, but the idea is you (or others) could easily run > from the workspace. Right? Yep. Anyone with JGit/EGit installed will get two extra tests executed. > What I tried was to select your AllRelengTests and > said "run as junit test" and got some (vague) error that some service could > not be obtained. I run it as a "JUnit Plug-in Test". If you're still getting errors, go to Run Configurations (from main toolbar). Find the config for AllRelengTests, switch to Plug-ins tab and go with either "Launch with all plugins" or "Add required plugins". FWIW, I'm on Eclipse 4.2.2. Let me know if you need any further assistance. > Much thanks for your persistence and continued support. Not a problem.
Note that LocalDiskRepositoryTest's import org.eclipse.jgit.storage.file.FileRepository; causes a compile error in master of JGit 3.0, since this class is no longer API: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=f32b8612433e499090c76ded014dd5e94322b786
(In reply to comment #34) > Note that LocalDiskRepositoryTest's > > import org.eclipse.jgit.storage.file.FileRepository; > > causes a compile error in master of JGit 3.0, since this class is no longer > API: > http://git.eclipse.org/c/jgit/jgit.git/commit/ > ?id=f32b8612433e499090c76ded014dd5e94322b786 @Tomasz, will you please fix the compile errors? My advice is to target EGit/JGit that's planned for Kepler (3.1, I think); Rather than try to have a version that "works with both old and new". This should be done in the next week to easily make it into M7. For an example of how this (might) be causing problems for people, see bug 405849.
Created attachment 230013 [details] GitCopyrightAdapterTest v05 Previous patch consuming the new JGit API.
(In reply to comment #36) > Created attachment 230013 [details] > GitCopyrightAdapterTest v05 > > Previous patch consuming the new JGit API. Thanks Tomasz. On the surface it looks right, so feel free to commit/push. But, I was not able to apply they patch, so if you want to let me in on the secret, that'd be good too.
Created attachment 230048 [details] GitCopyrightAdapterTest v06 (In reply to comment #37) > But, I was not able to apply they patch, so if you want to let me in on the > secret, that'd be good too. Sorry, my bad, forgot to pull before creating the patch.
I could apply this patch, and finally got my workspace cleaned up enough I could run unit tests, and the two tests passed. So, I think fine to commit. I couldn't really find a version of EGit 3.0 except from their repository, but hopefully they'll get a version in common repo for M7 in a few weeks. You are still releng committer, right? So, I think you can commit this yourself, if you'd like. (If you can't, or prefer me to, let me know). Thanks.
Having not heard anything, I went ahead and committed the latest patch. The reason being that I'll be doing some "test builds" today and Sunday we start "M7 stabilization" week, so want to be sure this is "in" and ready to go. http://git.eclipse.org/c/platform/eclipse.platform.releng.git/commit/?id=91528b760a2551ed9c297e039d708ddd2305e075
build failed. Guess I'm still confusing myself ... even though marked "optional", only optional at runtime ... needed for compile. And, so far, egit 3 doesn't exist in a repo ... at least the one we have in Eclipse parent POM. <egit-repo.url>http://download.eclipse.org/egit/updates</egit-repo.url>
(In reply to comment #40) > Having not heard anything, I went ahead and committed the latest patch. Sorry David, I was swamped last week. (In reply to comment #41) > And, so far, egit 3 doesn't exist in a repo ... Are you ok with what Matthias suggested to fix this [1]? [1] http://dev.eclipse.org/mhonarc/lists/egit-dev/msg03104.html
(In reply to comment #42) > (In reply to comment #40) > > Having not heard anything, I went ahead and committed the latest patch. > > Sorry David, I was swamped last week. > No problem. My impatience gets the best of me at times ... and we saw the consequences :) > (In reply to comment #41) > > And, so far, egit 3 doesn't exist in a repo ... > > Are you ok with what Matthias suggested to fix this [1]? > > [1] http://dev.eclipse.org/mhonarc/lists/egit-dev/msg03104.html Yep, we'll go with that we have for M7, and after that, change our egit pre-req repo to pick up "3.0" and add unit test then.
Ok, finally have a URL for 3.0. http://download.eclipse.org/egit/staging/v3.0.0.201305080800-m7 Assuming we get a good I-build tonight, I'll plan on changing this in our parent pom early Thursday, and try to do a "test build" before the official 8 PM I-build. The purpose of the test build isn't to test the unit test, just to make sure nothing odd breaks ... such as sometimes we have to "touch" features, to force their qualifiers to update, when we update "foreign" bundles.
I've applied the new (3.0) URL: http://git.eclipse.org/c/platform/eclipse.platform.releng.aggregator.git/commit/?id=8f64c015b980763554a996d8ae69868d51ac7643 and applied the patch. Hence, I'll mark as "fixed", though the URL will change as we go through RCs. I did confirm it compiled once I "updated" to the version at the new URL, but did not try running it
