Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 317715 - [qting-edge] qt4-build-edge.eclass is broken wrt to current git.eclass and checks out wrong branch
Summary: [qting-edge] qt4-build-edge.eclass is broken wrt to current git.eclass and ch...
Status: RESOLVED TEST-REQUEST
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Eclasses (show other bugs)
Hardware: All Linux
: High normal (vote)
Assignee: Qt Bug Alias
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-29 07:07 UTC by Marcel Partap
Modified: 2010-05-01 10:12 UTC (History)
3 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
fix, trivial (git-eclass-set-egit-commit-to-cursha1.patch,329 bytes, patch)
2010-04-29 07:08 UTC, Marcel Partap
Details | Diff
fix for qt4-build-edge (qt4-build-edge.patch,732 bytes, patch)
2010-04-29 13:16 UTC, Matthias Dahl
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel Partap 2010-04-29 07:07:13 UTC
When i wanted to check out qt-4.7.9999-stable-branch yesterday, not only did the build fail somewhere in the middle
> > > qdbusviewer.h:62: error: ISO C++ forbids declaration of ‘QDBusConnection’
> > > with no type qdbusviewer.h:62: error: expected ‘,’ or ‘...’ before ‘&’
> > > token
> > > qdbusviewer.h:75: error: ISO C++ forbids declaration of ‘QDBusMessage’
> > > with no type qdbusviewer.h:75: error: expected ‘,’ or ‘...’ before ‘&’
> > > token
> > > qdbusviewer.h:90: error: ‘QDBusConnection’ does not name a type
...what made me really suspicious was that it created files like /usr/lib64/qt4/libQtCore.so.4.8.0 and moc -v gave Qt Meta Object Compiler version 62 (Qt 4.8.0)  ..... Mmmhh .. git log on the work dir and such confirmed that indeed what was checkout was not the correct branch but master instead. It even says so at the beginning:

>   * GIT update -->
>   *    repository:               git://gitorious.org/qt/qt.git
>   *    at the commit:            db9bfd3653de23693db67ec5a0534e4d4ac97f7d
>   *    commit:                   master
>   *    branch:                   4.7-stable
>   *    storage directory:        "/usr/portage/distfiles/git-src/qt-4.7.9999"
Schizophrenic.. it says "at commit so-and-so", then prints and actually checks out master. Why? Because before checking actually out a branch, EGIT_COMMIT is not actually set to ${cursha1} of the correctly cloned tree, thus making git_branch fall back to 'master'. B0rk.

Reproducible: Always

Steps to Reproduce:
1. ACCEPT_KEYWORDS="**" emerge --color y -vd1 --nodeps =x11-libs/qt-core-4.7.9999 2>&1|grep -C2 -i commit
2. amaze
3. b0rk
Comment 1 Marcel Partap 2010-04-29 07:08:22 UTC
Created attachment 229621 [details, diff]
fix, trivial
Comment 2 Davide Pesavento gentoo-dev 2010-04-29 12:31:51 UTC
So do you think this is a bug in git.eclass?
Comment 3 Marcel Partap 2010-04-29 12:48:55 UTC
> So do you think this is a bug in git.eclass?
Well since you're asking this way, you seem not to think that.. at least adding that line solved my problem and checked out the correct tree tip..?
Comment 4 Matthias Dahl 2010-04-29 13:15:01 UTC
I actually beg to differ. Your approach will not fix the underlying problem but just fix one symptom of it. Please let me explain: The qt4-build-edge eclass does not set all required global EGIT_* variables prior to inheriting the git eclass. Thus some magic done in git eclass's global scope, won't set EGIT_COMMIT correctly (which will be set to master by default) and you end up with the wrong "branch". Since all variables are set _after_ inheriting, some things will work but eventually you get the wrong tree due to EGIT_COMMIT being set to master. Your approach simply works around that.

So qt4-build-edge needs fixing. I have already posted a small patch to the list and will attach it here.

Please, if I am wrong and embarrassed myself, tell me and I'll walk away in shame and apologize. :-)
Comment 5 Matthias Dahl 2010-04-29 13:16:16 UTC
Created attachment 229679 [details, diff]
fix for qt4-build-edge
Comment 6 Davide Pesavento gentoo-dev 2010-04-29 13:32:56 UTC
(In reply to comment #5)
> Created an attachment (id=229679) [details]
> fix for qt4-build-edge
> 

This patch will solve the issue but IMHO it's rather ugly. Does git eclass really need to do its magic in the global scope? I don't think so...
Comment 7 Markos Chandras (RETIRED) gentoo-dev 2010-04-29 13:47:59 UTC
cc'ing git.eclass maintainers

Comment 8 Matthias Dahl 2010-04-29 14:30:40 UTC
The ugliness comes from the fact that qt4-build-edge uses the git eclass in a way it is actually not supposed too and no other ebuild does it that way. ;-)

That said, I nevertheless think that said magic come safely be pushed into git_fetch() (EGIT_BRANCH  and EGIT_COMMIT) or partly into git_fetch() (EGIT_BRANCH) and git_branch() (EGIT_COMMIT) without doing any harm. IMHO that would be the best thing to do and make the eclass a bit more flexible for eclass maintainers who try to use it.
Comment 9 Markos Chandras (RETIRED) gentoo-dev 2010-04-29 14:40:27 UTC
I am having difficulties understanding the origin of the problem. Could somebody please elaborate in details so I can understand where is the problem?
Comment 10 Tomáš Chvátal (RETIRED) gentoo-dev 2010-04-29 14:47:18 UTC
You have to define all required EGIT variables before inheriting git eclass.
Other way around you are just asking for trouble.

Specialy when egit_commit and egit_branch are used.
Comment 11 Davide Pesavento gentoo-dev 2010-04-29 14:51:01 UTC
(In reply to comment #8)
> The ugliness comes from the fact that qt4-build-edge uses the git eclass in a
> way it is actually not supposed too and no other ebuild does it that way. ;-)
> 

No. The ugliness is caused by some limitations imposed by git.eclass's public interface.

> That said, I nevertheless think that said magic come safely be pushed into
> git_fetch() (EGIT_BRANCH  and EGIT_COMMIT) or partly into git_fetch()
> (EGIT_BRANCH) and git_branch() (EGIT_COMMIT) without doing any harm. IMHO that
> would be the best thing to do and make the eclass a bit more flexible for
> eclass maintainers who try to use it.
> 

Exactly my thoughts.
Comment 12 Davide Pesavento gentoo-dev 2010-04-29 14:56:42 UTC
(In reply to comment #10)
> You have to define all required EGIT variables before inheriting git eclass.

(BTW, this is not documented)

> Other way around you are just asking for trouble.
> 
> Specialy when egit_commit and egit_branch are used.
> 

Why? Can't we just move EGIT_COMMIT initialization to the beginning of git_fetch()?
Comment 13 Tomáš Chvátal (RETIRED) gentoo-dev 2010-04-29 15:04:16 UTC
(In reply to comment #12)
> Why? Can't we just move EGIT_COMMIT initialization to the beginning of
> git_fetch()?
> 

I said multiple times, git eclass variables are global and required to be defined before inherit.
Sadly none of you guys ever listened to me. Also i said that i will gladly leave maintianership to anyone else willing to work for it and implement any changes they want to do.
Comment 14 Markos Chandras (RETIRED) gentoo-dev 2010-04-29 15:18:33 UTC
Couldn't we just inherit git eclass in the middle of our eclass when we have defined all the required variables? Ugly but it might work 
Comment 15 Matthias Dahl 2010-04-29 17:46:46 UTC
(In reply to comment #14)
> Couldn't we just inherit git eclass in the middle of our eclass when we have
> defined all the required variables? Ugly but it might work 

Unfortunately not. Inheriting is only allowed during the depend phase and will produce _at least_ a QA warning otherwise or worse.

(In reply to comment #13)
> I said multiple times, git eclass variables are global and required to be
> defined before inherit.

This is not meant as criticism, just a suggestion: It would be nice to have this information as a one liner in the eclass's header. I mean it's easy to figure out once you had a brief look through the eclass but the average joe just won't do that... so wouldn't I by the way.

> Sadly none of you guys ever listened to me. Also i said that i will gladly
> leave maintianership to anyone else willing to work for it and implement any
> changes they want to do.

Currently only EGIT_COMMIT and EGIT_BRANCH are really required during the depend phase, everything else can actually be defined later. So pushing those related lines down to git_fetch() wouldn't hurt as far as I could tell and would lift the requirement to have really everything defined before inheriting the eclass. Otherwise there is hardly a way around the problem because if you depend on USE flags or similar to figure out the correct branch, there is not much you can do as you cannot check the flags during the depend phase (which I learnt just a few days ago -g-).
Comment 16 Matthias Dahl 2010-04-29 17:50:51 UTC
One more thing: I do understand why Tomáš put those lines in global scope. IMHO it is actually where they belong and makes things clearer/easier for future additions to the eclass where you could end up with several places where you would need to do exactly that same stuff and would have to workaround it with an init function and alike.
Comment 17 Markos Chandras (RETIRED) gentoo-dev 2010-04-29 18:32:26 UTC
(In reply to comment #16)
> One more thing: I do understand why Tomáš put those lines in global scope.
> IMHO it is actually where they belong and makes things clearer/easier for
> future additions to the eclass where you could end up with several places where
> you would need to do exactly that same stuff and would have to workaround it
> with an init function and alike.
> 
I am confused again. Does the latest attachment actually solves the buggy? I really hope so cause touching all the live ebuilds is a NO go
Comment 18 Matthias Dahl 2010-04-29 19:08:51 UTC
(In reply to comment #17)

The latest patch (qt4-build-edge.patch) works around the underlying problem, yes. And can IMHO be safely committed.
Comment 19 Marcel Partap 2010-04-29 21:27:53 UTC
well.. so what is wrong with my patch? why isn't that the correct solution? Surely if you define just EGIT_BRANCH, it shouldn't clone the right branch just to then check out a different one? This is wrong eclass behavior imho.
Comment 20 Matthias Dahl 2010-04-30 06:43:40 UTC
(In reply to comment #19)

> well.. so what is wrong with my patch? why isn't that the correct solution?

Unfortunately, it covers not all execution paths. You just covered one of several if branches. Also it does not fix the problem if ones wants to checkout a very specific tree state (EGIT_COMMIT). I haven't looked closely but it might even break this functionality for other git eclass users.

> Surely if you define just EGIT_BRANCH, it shouldn't clone the right branch
> just to then check out a different one? This is wrong eclass behavior imho.

That's actually not the case. The git eclass has some usage guidelines, one of them being that all EGIT_* variables have to be defined _prior_ to inheriting the eclass. If you do not adhere to that guideline, you are on your own... which is fine by the way. Breaking the rules is never a good idea. And that is exactly what is happening w/ the qt4-build-edge eclass. It simply needs to define the EGIT_* variables after inheriting the git eclass and thus those problems come up. So every other git eclass user who stays true to the guidelines won't run into this sort of problems.

I hope that cleared it up.
Comment 21 Markos Chandras (RETIRED) gentoo-dev 2010-04-30 08:47:15 UTC
At this point we don't quite care if somebody wants to checkout a very specific commit hash. I would be glad if we could come up with a patch that actually can clone the latest commit hash from each branch. Matthias proposed a workaround which can easily be applied to the rest of the branches as well
Comment 22 Matthias Dahl 2010-04-30 08:59:46 UTC
(In reply to comment #21)

> At this point we don't quite care if somebody wants to checkout a very
> specific commit hash.

But I do think that is important functionality especially if you are looking for a bug or upstream is broken.

> I would be glad if we could come up with a patch that actually can
> clone the latest commit hash from each branch. Matthias proposed a workaround
> which can easily be applied to the rest of the branches as well

Now I don't quite understand what you mean. :-) My patch works around the problem and fixes it for all qt live ebuilds, no matter if 4.7 or 4.6 or stable or kde. Sorry for asking but what branches are you talking? -confused :)-

The other way would be to push some stuff in git.eclass out of the global scope and down into git_fetch(). But that's not my decision to make.
Comment 23 Matthias Dahl 2010-04-30 09:02:28 UTC
I took the liberty to change the bug summary to reflect the real cause. I hope this was okay.
Comment 24 Markos Chandras (RETIRED) gentoo-dev 2010-04-30 09:48:23 UTC
(In reply to comment #22)
> (In reply to comment #21)
> 
> > At this point we don't quite care if somebody wants to checkout a very
> > specific commit hash.
> 
> But I do think that is important functionality especially if you are looking
> for a bug or upstream is broken.
> 
Sure if you are doing a bug hunting :-). I am still trying to figure out a way to make EGIT_COMMIT usable again
> > I would be glad if we could come up with a patch that actually can
> > clone the latest commit hash from each branch. Matthias proposed a workaround
> > which can easily be applied to the rest of the branches as well
> 
> Now I don't quite understand what you mean. :-) My patch works around the
> problem and fixes it for all qt live ebuilds, no matter if 4.7 or 4.6 or stable
> or kde. Sorry for asking but what branches are you talking? -confused :)-
> 
Yes thats true. I misunderstood the code
Comment 25 Matthias Dahl 2010-04-30 11:03:57 UTC
(In reply to comment #24)

> Sure if you are doing a bug hunting :-). I am still trying to figure out a
> way to make EGIT_COMMIT usable again

We two do definitely have some kind of communication problem. :-) Again, I am not really sure what it is you are talking about. With my patch, EGIT_COMMIT will actually work like it is supposed to- again, with all qt live ebuilds that is.

Or is there something I am missing...?
Comment 26 Markos Chandras (RETIRED) gentoo-dev 2010-04-30 11:16:05 UTC
hehe. as far as I understand your patch only clones the latest commit hash and you cant override it. If you do bug hunting you might want to checkout another commit hash and *not* the latest one
Comment 27 Matthias Dahl 2010-04-30 11:41:39 UTC
(In reply to comment #26)

> hehe. as far as I understand your patch only clones the latest commit hash
> and you cant override it.

Nope, that's not the case actually. If you are looking at the right patch (qt4-build-edge.patch), you'll find that I save the users EGIT_COMMIT and reapply it later on if it was set, otherwise I set EGIT_COMMIT to the correct branch name. That is what git.eclass would have done for us, if we provided our EGIT_* variables prior to inheriting it.

By the way, I have naturally tested the patch prior to posting it, so I can say with 100% certainty that it works just fine and is correct as far as possible wrt our git.eclass guidelines violation. :-)

> If you do bug hunting you might want to checkout another commit hash and
> *not* the latest one

I totally agree. :-) That's what I said all along, actually. ;)

Let's blame our communication problem on sudden solar flares or something along those lines. :-)))
Comment 28 Ben de Groot (RETIRED) gentoo-dev 2010-04-30 23:51:25 UTC
If I may pitch in while officially away, it seems to me that the problem is that qt4-build-edge.eclass needs more flexibility than git.eclass offers. So why not fix this at the source (git.eclass) rather than applying a workaround?
Comment 29 Markos Chandras (RETIRED) gentoo-dev 2010-05-01 08:17:35 UTC
(In reply to comment #28)
> If I may pitch in while officially away, it seems to me that the problem is
> that qt4-build-edge.eclass needs more flexibility than git.eclass offers. So
> why not fix this at the source (git.eclass) rather than applying a workaround?
> 

I don't quite understand why git eclass does all the magic in global scope ( by the way this behavior changed recently right? ). In any case, since the attached patch works for us we can use it and stop bothering with the git.eclass
Comment 30 Matthias Dahl 2010-05-01 08:51:31 UTC
I'd suggest the following.

I agree w/ Ben that in the long run git.eclass should be extended to be more flexible. For that we have to properly evaluate who uses git.eclass and what can be do and what can't be do to stay compatible w/ all the current users because IMHO that is a very important point.

In the short term we should fix qt4-build-edge asap because currently it is broken and we cannot expect normal users to fix it by themselves. So applying the workaround is not ideal but fixes the symptoms and buys us time to look into git.eclass w/o any pressure. That does _not_ mean we should just forget about it- a workaround is a workaround and not a proper fix. A few changes to git.eclass could lead to breakage again.

(In reply to comment #28)
> I don't quite understand why git eclass does all the magic in global scope (
> by the way this behavior changed recently right? ).

Actually, it is understandable from my pov. If git.eclass gets extended some day and a situation arises where there is no linear execution path given any more but there are branches that are or are not mutual exclusive, it is hard to properly place such initializations w/o pollution everything unnecessarily.

IMHO placing such initializations in pkg_setup() would be the ideal point. git.eclass would be more flexible because nothing is done in global scope and it would be more future proof than pushing it down into git_fetch(). Unfortunately it would also break some ebuilds and eclasses because they naturally don't have a call to it yet. So that solution is not very ideal from a compatibility pov.

@Tomáš Chvátal: What do you think?

> In any case, since the attached patch works for us we can use it and stop
> bothering with the git.eclass

see the beginning of this posting please :-)
Comment 31 Matthias Dahl 2010-05-01 08:54:20 UTC
Obviously that should have been "can be done and can't be done". And "pollution" should have been "polluting". Oh well. :-(
Comment 32 Markos Chandras (RETIRED) gentoo-dev 2010-05-01 10:12:02 UTC
Patch applied

Please test and reopen this bug if needed

Thank you all for your contribution