Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

app-office/libreoffice: fix ebuild bug building with clang #2145

Closed
wants to merge 1 commit into from
Closed

app-office/libreoffice: fix ebuild bug building with clang #2145

wants to merge 1 commit into from

Conversation

uxcn
Copy link
Contributor

@uxcn uxcn commented Aug 25, 2016

This fixes a bug that misidentifies clang as gcc in a compiler test.

Gentoo-Bug: 598948
Suggested-By: Jason Schulz jason@schulz.name

@gktrk
Copy link
Member

gktrk commented Aug 25, 2016

Can you double check the bug number?

@gktrk
Copy link
Member

gktrk commented Aug 25, 2016

@akhuettel @scarabeusiv

@gktrk gktrk added bugfix assigned PR successfully assigned to the package maintainer(s). labels Aug 25, 2016
@@ -265,7 +265,7 @@ pkg_pretend() {
if [[ ${MERGE_TYPE} != binary ]]; then
check-reqs_pkg_pretend

if [[ $(tc-getCC) == clang ]] ; then
if [[ $(tc-getCC) == *clang* ]] ; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of just testing for clang and then using the else code path, test only for GCC using the new tc-is-gcc function, i.e.

if tc-is-gcc; then
    # gcc blabla
fi

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd much rather check for the exception on top (clang being used) and then have it default to GCC for whatever may go wrong there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been impacting users... including me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you wish to test for clang, then use tc-is-clang instead, preferably by negating it and attaching it to the elif expressions. Don't match names.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while you're at it, please extend the fix to all libreoffice ebuilds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still borked in the latest git pull.

Copy link
Member

@a17r a17r Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to be credited with the fix, then leave this PR open, but as I said please also extend it to the other ebuilds. after all you don't want that bug re-introduced when the next libreoffice version bump is created out of an unfixed live ebuild. At the same time change to use tc-is-clang as suggested in https://github.com/gentoo/gentoo/pull/2145/files#r76622697

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@a17r Who maintains the libreoffice ebuilds? I fixed the other ebuilds for you, but this is as much time as I have to spend on this problem. At least two people are still waiting for this to be fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

github is not just an alternative place to bugzilla to voice your concerns, this is where you get involved yourself to help. The (already existing) bug is enough to make maintainers aware of the issue, if that's what you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a week since I submitted a patch on this. That's longer than a week that people have been having issues with it. I am truly sorry, but I can't afford any more time on this.

I sincerely hope someone else fixes it.

On Wed, Aug 31, 2016 at 11:46:24AM -0700, Andreas Sturmlechner wrote:

@@ -265,7 +265,7 @@ pkg_pretend() {
if [[ ${MERGE_TYPE} != binary ]]; then
check-reqs_pkg_pretend

  •   if [[ $(tc-getCC) == clang ]] ; then
    
  •   if [[ $(tc-getCC) == _clang_ ]] ; then
    

github is not just an alternative place to bugzilla to voice your concerns, this is where you get involved yourself to help. The (already existing) bug is enough to make maintainers aware of the issue, if that's what you want.

You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
https://github.com/gentoo/gentoo/pull/2145/files/6992b9b78310bb8769a490a6deeaee1bf23aeb45#r77048658

@uxcn
Copy link
Contributor Author

uxcn commented Aug 26, 2016

Sorry @gktrk, I transposed two digits. It should be 589948.

Gentoo-Bug: 589948
Suggested-By: Jason Schulz <jason@schulz.name>
if [[ $(tc-getCC) == clang ]] ; then
: # ignore clang, which works
elif [[ $(gcc-major-version) -lt 4 ]] || {
if [[ $(gcc-major-version) -lt 4 ]] || {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't look right, you basically reverted 2ed0f7a and got rid of the check that was supposed to fix build with clang in the first place (even if it didn't work).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@a17r Thanks, I'm closing the pull request.

@uxcn uxcn closed this Aug 31, 2016
@SoapGentoo
Copy link
Member

@uxcn While I do understand your frustration with the whole process dragging on, we can't just accept random drive-by patches. If you want to contribute, certain minimum QA standards have to be fulfilled.

@a17r
Copy link
Member

a17r commented Aug 31, 2016

Dealing with it in existing PR now: #1907

@uxcn
Copy link
Contributor Author

uxcn commented Aug 31, 2016

@SoapGentoo I appreciate the dilemma, I just don't have the time to spare on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s).
Projects
None yet
4 participants