Community
Participate
Working Groups
As part of the work to move from CVS to Git (bug 345479) we need to ensure the RelEng Tools Copyright Updater works correctly against a Git repository.
I can verify that the copyright checker works ok against Git projects. The only missing thing is that it doesn't check the commit history to determine the appropriate year to write in the copyright. It instead gracefully falls back to the current year. Obviously computing the correct year from the Git history would be useful.
(In reply to comment #1) > I can verify that ... I meant to say, I *have* verified that...
*** Bug 349243 has been marked as a duplicate of this bug. ***
I'd assume that it also doesn't check whether the last commit comment had the String "copyright" in it indicating that a manual fixup was made and thus no change of copyright is needed.
(In reply to comment #4) > I'd assume that it also doesn't check whether the last commit comment had the > String "copyright" in it indicating that a manual fixup was made and thus no > change of copyright is needed. John, did you test that? This one is a must fix for me.
(In reply to comment #4) > I'd assume that it also doesn't check whether the last commit comment had the > String "copyright" in it indicating that a manual fixup was made and thus no > change of copyright is needed. Right. Since it is not git-aware it doesn't search commit comments like this yet.
Tomasz, are you still working on this, if so the target milestone needs to be moved.
(In reply to comment #7) > Tomasz, are you still working on this, if so the target milestone needs to be > moved. Tomek planned to start working on it late in M5 and continue in M6. So the feature should be added during M6. Changing the target.
A fix and tests have been pushed to Gerrit as https://git.eclipse.org/r/#/c/5223/ and https://git.eclipse.org/r/#/c/5224/ respectively.
Created attachment 212139 [details] GitCopyrightAdapter The fix adds dependencies on org.eclipse.jgit and org.eclipse.egit.core
Created attachment 212140 [details] GitCopyrightAdapter - tests This one also adds deps on JGit/EGit but also bumps Java to 1.5. Not sure if this is ok.
These are basically the same patches as in https://git.eclipse.org/r/#/c/5223/ and https://git.eclipse.org/r/#/c/5224/ from EGit/PDE. I'm a Platform/Releng committer, but still would like to have +1 from someone else.
Hold your horses! Even though the test pass, when I ran the action on eclipse.platform.team it touched all the files. So there is obviously something wrong with it. Investigating...
(In reply to comment #10) > > The fix adds dependencies on org.eclipse.jgit and org.eclipse.egit.core I think the new dependency makes sense. But there will likely need to be a builder change to make sure the required eGit bundles are fetched at build-time. If it helps here is the map entry from Orion's build: plugin@org.eclipse.jgit=p2IU,id=org.eclipse.jgit,repository=http://download.eclipse.org/egit/updates plugin@org.eclipse.egit.core=p2IU,id=org.eclipse.egit.core,repository=http://download.eclipse.org/egit/updates This pulls from eGit's release update site. I think we should avoid using eGit nightly builds and using unreleased API if possible.
(In reply to comment #14) > I think we should avoid using eGit > nightly builds and using unreleased API if possible. Totally agree, I sticked to stable 1.3.0 in both patches.
Created attachment 212145 [details] GitCopyrightAdapter v02
Created attachment 212146 [details] mylyn/context/zip
Created attachment 212147 [details] GitCopyrightAdapter - tests v02
Now, this time the updater will work as expected.
Looks good Tomasz. I think you can go ahead and release. I tried it out on a couple of projects and manually verified the results. I think we should let people on the team know it is available, but suggest they carefully check the results at first to make sure it looks reasonable. Since this change only affects the date calculation logic, and not the code that actually modifies the files, the worst case is that it inserts the wrong date rather than mangling files.
Thanks for looking at this. The only missing bit now is the required builder change due to the new dep on JGit and EGit/Core (comment 14). I know that Kim is extremely busy right now, so if she doesn't find time to comment how to update the builder I will have a look at it next week.
David, could you please comment on the required builder change to make sure it doesn't blow up missing new dependencies on JGit/EGit bundles.
Let's worry about this one after EclipseCon. We're having enough trouble just getting our builds going right now.
I made a pass on core.resources and core.tests.resources. I couldn't find any false positives or undiscovered changes. It works good.
I ran it against all projects from eclipse.platform.compare. It resulted in 200 changes: * 1 valid fix for /org.eclipse.team.ui/src/org/eclipse/team/internal/ui/history/WorkbenchHistoryPageSite.java * 2 suggested fixes with new copyright sections and year 2012. I don't think 2012 is the best choice here. ** /org.eclipse.core.net/natives/unix/gnomeproxy.h ** /org.eclipse.core.net/natives/win32/jWinHttp.h * 2 suggested fixes with new copyright sections and year 2012 for dummy files in tests. ** /org.eclipse.team.tests.cvs.core/resources/CommandTest/project3/src/file.c ** /org.eclipse.team.tests.cvs.core/resources/CommandTest/project3/src/file.h * 195 changes for the f0e53ee2640b02d1f95f87dfcfa88331c3336845 move made as part of bug 359032. What do you think about ignoring the last two (dummy files and compare move)? Other than that looks good.
David any news on the required builder change (comment 21)? Can I be of any help?
(In reply to comment #26) > David any news on the required builder change (comment 21)? Can I be of any > help? Well, now as comment 21 says, "Kim" is still very busy Where Kim is me. :) My first priorities are getting the builds running and then unit tests running ... but, while I'm waiting for a test build to finish ... You can help by spelling out to me some of the "big picture" ... This copyright tool used to be in "org.eclipse.releng.tools" bundle, right? The patches appear to be in a git repository? I've heard this "releng tool" is supposed to "move back" to platform ... has that happened yet? Or, am I way off base? Second, does/is Eclipse SDK making egit/jgit available as part of the "Eclipse SDK"? Like they do CVS? I seem to recall hearing they would, but ... don't see them included, so just wondering what/why the conclusion was? Depending on the answers to those questions, would lead to the next one ... are you looking to _package_ egit/jgit with the tool? Or, just have it available during the build, but not specifically not "included" with the tool itself. Thanks,,
(In reply to comment #27) > This copyright tool used to be in "org.eclipse.releng.tools" bundle, right? Yes. > The patches appear to be in a git repository? I've heard this "releng tool" is > supposed to "move back" to platform ... has that happened yet? Or, am I way off > base? The plan is to commit the patch to org.eclipse.releng.tools in the platform releng repository. > Second, does/is Eclipse SDK making egit/jgit available as part of the "Eclipse > SDK"? Like they do CVS? I seem to recall hearing they would, but ... don't see > them included, so just wondering what/why the conclusion was? No. We are not including EGit in the SDK or any of our zips. See discussion in http://wiki.eclipse.org/Platform-releng/Juno_Git_Migration#Including_eGit_in_Eclipse_SDK > Depending on the answers to those questions, would lead to the next one ... are > you looking to _package_ egit/jgit with the tool? Or, just have it available > during the build, but not specifically not "included" with the tool itself. No. The releng tools bundle would simply require JGit. You would need to either install JGit or p2 could install the JGit bundle at the time the releng tools plugin is installed. The only releng issue here is that there is now a build-time dependency on JGit. So, the builder needs to fetch JGit and have it available in the target at build time. We would not include JGit in any feature, zip, or p2 repo coming out of the build.
Ok, I've made the initial changes to the builder so egit and jgit will be present during the build ... and, I'd say there's a fair chance of it working! But, reviewing the patch in comment 16, I can make the following observations: 1) You essentially "require bundle" in the manifest.mf file org.eclipse.jgit;bundle-version="1.3.0", org.eclipse.egit.core;bundle-version="1.3.0" I think you'd need those "optional", in order to NOT "pull them in" to the releng tool repo. I suspect conceptually it makes sense to have them optional ... if someone was using the tool with only CVS, say, then they would not need the git bundles? If they can't be optional, I don't think there is any great harm (perhaps some advantage) ... if someone already had them installed, or had something higher installed, it would not use the ones from our little repo ... there is no "tight coupling", we just happen to have a version there handy. 2) Also, you have Export-Package: ... org.eclipse.releng.tools.git, You'd know better than I, but is that really an "API package" we want to expose to the world to use? I hope these review comments are helpful. I set up the build to use the repo at http://download.eclipse.org/egit/updates/ And simply get the "latest", which is currently org.eclipse.egit.core_1.3.0.201202151440-r.jar org.eclipse.jgit_1.3.0.201202151440-r.jar Have I read this right ... that you, Thomas, will apply the patch? (And, be around to revert it, if something goes wrong :)
Created attachment 214376 [details] GitCopyrightAdapter v03 (In reply to comment #29) > 1) You essentially "require bundle" in the manifest.mf file > > org.eclipse.jgit;bundle-version="1.3.0", > org.eclipse.egit.core;bundle-version="1.3.0" > > I think you'd need those "optional", in order to NOT "pull them in" to the > releng tool repo. I suspect conceptually it makes sense to have them optional AFAIK, a missing, "optional" dependency doesn't prevent the plug-in to run. Not sure what you meant by '"pull them in" to the releng tool repo'. > ... if someone was using the tool with only CVS, say, then they would not need > the git bundles? If they can't be optional, I don't think there is any great > harm (perhaps some advantage) ... That would be a fair argument if CVS repos were majority. I would flip your comment and suggest to consider making the CVS dep optional ;) > if someone already had them installed, or had > something higher installed, it would not use the ones from our little repo ... Please correct me if I'm wrong, but a higher version will be used, no matter if the dependency is marked as "optional" or not. I did apply your suggestion to the newest patch though. > 2) Also, you have > > Export-Package: ... > org.eclipse.releng.tools.git, > > You'd know better than I, but is that really an "API package" we want to expose > to the world to use? I thought that all packages from the releng tool should be exported. You're right, it doesn't make much sense to make the "git" package visible. I would keep it exported but hidden (internal) at the same time. > Have I read this right ... that you, Thomas, will apply the patch? (And, be > around to revert it, if something goes wrong :) Yes, that's going to be me. Does it mean I have the green light and can push the change? PS. Any comments on tests (comment 18)? Dependency on org.eclipse.jgit.junit seems to be an issue there.
(In reply to comment #30) > AFAIK, a missing, "optional" dependency doesn't prevent the plug-in to run. > Not sure what you meant by '"pull them in" to the releng tool repo'. At some point in the build process, we "say" "now that things are compiled and made into bundles, let's put them into a p2 repo" (and those one big one of course, but even smaller ones, sometimes, that we zip up and make available for download, as we do the releng tool. And those repos are part of our "deliverables"). So, if not optional, the "make a repo" step will pick them up both for our main big repo and the little "releng tool only" archived repo; in the optional case not picked up for the repos (well, usually, guess that is controllable by exactly how we make the repo, with I'd say I'm only 90% sure of. I think either case is not so bad. Just wanted to be sure we were explicit one way or the other. And having them non-optional, and exist also in our repo (as well as the normal, original EGit repo) might make the testing part go easier :) ... I have not looked at that part, and not sure what else would have to change, but it's pretty normal to have a "git all prereqs" sort of step in the tests which is where we'd add the "get git" part. But, I'd suggest you add them today, we'll see if things blow up, and we'll figure out the tests later. (at worst, the tests would fail ... and right now, lots of them are :( [when they run at all, which is just starting to get working again.]. > That would be a fair argument if CVS repos were majority. I would flip your > comment and suggest to consider making the CVS dep optional ;) > True, I guess each should be optional (but, not sure I'd rock the existing boat, at least right now). > I thought that all packages from the releng tool should be exported. You're > right, it doesn't make much sense to make the "git" package visible. I would > keep it exported but hidden (internal) at the same time. I believe that is "the platforms" normal policy (to make all visible, but mark internal if not API (but, if it was me, I might be a radical and deviate from that rule :) especially since the tool as a whole (as far as I know) is not meant to provide API. But your approach does sounds fine. > Yes, that's going to be me. Does it mean I have the green light and can push > the change? I can't wait! :) Seriously. Green light. release today ... I suspect I'll be doing some "test builds" during the day so we'll get an early look if it needs to be reverted before the I build in the morning. (That is, if I haven't really fixed the build right ... but 95% sure). > PS. Any comments on tests (comment 18)? Dependency on org.eclipse.jgit.junit > seems to be an issue there. Appreciate the reminder. I suspect I (we?) have to change something somewhere, but still learning a lot about the tests. As I mentioned, above, in the worst case, the test will fail, and that will remind me/us to fix the scripts that run the tests :) I'd go ahead and commit the tests, let the tests fail, if they do, and if it looks like it will "be a while" before I can fix that part, we'll open a separate bug, and disable the unit tests for M7 or something.
Thanks for the quick comment David. I've pushed both patches: * GitCopyrightAdapter as 9a580bf2137e0e71608a0951959092d51cf22692. Kept deps optional, changed the x-internal directive for o.e.releng.tools.git to x-friends:="org.eclipse.releng.tests". Tests require the package in order to run, that is why I exported it, now I remember :) * Tests for the copyright adapter as 1580002ad6b1358e26366718f8d5415a7686fd33. As in the patch, may fail: J2SE-1.5, dep on org.eclipse.jgit.junit... Decided to go with two commits in case the latter needs to be reverted. Feel free to ping me / file a bug and assign/CC me if needed. Keeping my fingers crossed.
What feature/repository is the bundle in? I tried a test N build, and its seemed like it was getting to "old" version ... I'm wondering if this is one of that that has to be "released" to both 'integration' and R4_HEAD? (Or, could be a problem with the N build, for all I know).
(In reply to comment #33) > What feature/repository is the bundle in? The bundle is in ssh://{user}@git.eclipse.org/gitroot/platform/eclipse.platform.releng.git. I install it by adding/updating "org.eclipse.releng.tools.feature.group" IU to my profile. The latest in the n-build repo is 3.4.101.N20120321-2000-xxxx, but it gives me an error saying there is no adapter for GitProvider when run against a git project. > I'm wondering if this is one of that that has to be "released" to both 'integration' and R4_HEAD? I've pushed both changes to the forementioned repo which currently has a single branch called "master".
Thomas, can you revert the "unit test" part? I was mistaken. It did cause the build to fail, not just a failed test. generateScript: [eclipse.buildScript] Some inter-plug-in dependencies have not been satisfied. [eclipse.buildScript] Bundle org.eclipse.releng.tests: [eclipse.buildScript] Missing required plug-in org.eclipse.jgit.junit_1.3.0. I'd leave in the code part. I am not sure where the org.eclipse.jgit.junit is? I didn't seem to see it in the egit release repo.
(In reply to comment #35) > Thomas, can you revert the "unit test" part? Done, see a7f02501b9f9e5f0ae1febc52f02904da9f35125. > I was mistaken. It did cause the build to fail, not just a failed test. Sorry to hear that. > I am not sure where the org.eclipse.jgit.junit is? I didn't seem to see it in > the egit release repo. Neither do I. I guess it's one of the test plug-ins which are not part of the o.e.jgit.feature.group.
(In reply to comment #36) > (In reply to comment #35) > > > I am not sure where the org.eclipse.jgit.junit is? I didn't seem to see it in > > the egit release repo. > > Neither do I. I guess it's one of the test plug-ins which are not part of the > o.e.jgit.feature.group. I suggest you confirm the releng tool being produced by I-builds works as you expect it to, and if so, let's close this one as fixed, then open a new one (or clone this one) to add unit test at a later time. It'll take some study to figure out what/where org.eclipse.jgit.junit is and how to get it into our build and tests. (Just in case, is that bundle _essential_ to your tests? Is there a possibility you could copy a class or two into your unit test bundles to suffice? I think that's allowable, as long as the original copyright header is left intact and you just add a comment "This originally came from xyz package from abc bundle". Just thought I'd mention it, since I've seen other cases (on other projects) where people had funny unit test dependencies for some fairly minor reasons. If minor, we should avoid them. If they really provide some complex service you just plugin into then that's a different matter and we should track in a longer term bug.). Thanks.
(In reply to comment #37) > I suggest you confirm the releng tool being produced by I-builds works as you > expect it to, and if so, let's close this one as fixed Works fine in I20120429-2000 + RelEng 3.6.100.v20120423-1453. > , then open a new one (or > clone this one) to add unit test at a later time. It's bug 378047. > It'll take some study to figure out what/where org.eclipse.jgit.junit is and > how to get it into our build and tests. (Just in case, is that bundle > _essential_ to your tests? Is there a possibility you could copy a class or two > into your unit test bundles to suffice? Yes, it can work out. I moved the discussion to bug 378047, I'm closing this one as fixed since we have the updater in place.
Tomasz, the tool doesn't quite work for SWT (way too many changes). I have opened bug 381176 for this.
FYI: The tool also has bug 215224, but that was also present in the CVS version.
