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

inkscape: fix build in High Sierra #895

Closed
wants to merge 1 commit into
base: master
from

Conversation

7 participants
@danchr
Contributor

danchr commented Oct 6, 2017

The current version of Inkscape fails with a build error due to an
ambigious call to abs(). Indeed, i->pathNext->id.objID is of type
unsigned int, for which abs() does not make much sense. Adding a cast
to 'int' fixes it.

(Suggested by Michael Lass)

Fixes: https://trac.macports.org/ticket/54886

Description
Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.12
Xcode 9.0

Verification

Have you

  • checked your Portfile with port lint?
  • tested basic functionality of all binary files?

@danchr danchr added the type: bugfix label Oct 6, 2017

@danchr danchr requested a review from dbevans Oct 6, 2017

@macportsbot

This comment has been minimized.

macportsbot commented Oct 6, 2017

Notifying maintainers:
@dbevans for port inkscape.

@cjones051073

This comment has been minimized.

Contributor

cjones051073 commented Oct 6, 2017

why not just remove the abs(...) call completely, given its pointless when called on an unsigned value...

@michaellass

This comment has been minimized.

Contributor

michaellass commented Oct 6, 2017

This PR does not solve the issue as far as I can tell. We have to convert both values before subtraction as otherwise the result of the subtraction is still unsigned.

Minimal example showing all the different variants and also why abs() is required at all:

#include <stdlib.h>
#include <stdio.h>

int main() {
  unsigned int a = 2;
  unsigned int b = 4;
  printf("Difference: %u\n", a-b);
  printf("Difference: %u\n", abs(a-b));
  printf("Difference: %u\n", abs((int)a-b));
  printf("Difference: %u\n", abs((int)a-(int)b));
}

Result:

Difference: 4294967294
Difference: 2
Difference: 2
Difference: 2

The first variant is clearly not what we want. The other ones create the desired result but variants two and three include an abs(unsigned int) which causes the error during compilation of inkscape.

@danchr

This comment has been minimized.

Contributor

danchr commented Oct 6, 2017

This PR does not solve the issue as far as I can tell. We have to convert both values before subtraction as otherwise the result of the subtraction is still unsigned.

Well, I updated the patch to use a cast to long rather than int as the latter still didn't compile. Please also note that the issue isn't the correctness of the code, but that the build is broken. I would suggest applying this interim fix, and later replace it with any better fix the Inkscape developers might apply.

FWIW this C++ code:

#include <stdlib.h>
#include <stdio.h>

int main() {
  unsigned int a = 2;
  unsigned int b = 4;
  printf("Difference: %u\n", a-b == 2);
#ifndef __clang__
  printf("Difference: %u\n", abs(a-b) == 2);
#endif
  printf("Difference: %u\n", abs((long)a-b) == 2);
  printf("Difference: %u\n", abs((long)a-(long)b) == 2);
}

…yields the same* results with GCC on Ubuntu and Clang on my Mac. So any bug in this code is likely to affect all platforms.

(* With the obvious difference that the Clang compile omits the line that doesn't compile.)

@michaellass

This comment has been minimized.

Contributor

michaellass commented Oct 6, 2017

Indeed it works by casting only the first element to long as well. Interesting. Brief summary:

  • Just leaving out abs() would change behavior which we clearly do not want
  • abs((int)a-b) does not compile
  • abs((long)a-b) is a valid fix
  • abs((int)a-(int)b) is a valid fix as well

So this PR looks fine indeed. By the way: I looked into the current master branch of inkscape and the line of code is gone. Still, upstream may be interested in this.

@danchr

This comment has been minimized.

Contributor

danchr commented Oct 6, 2017

Indeed it works by casting only the first element to long as well. Interesting.

Without having looked into the intricacies of the standards, I'd presume it's caused by the long “winning” over the unsigned int, and thus coercing the entire statement to a signed type. Two casts might well be ever so slightly faster, but I doubt it'd matter…

platform darwin 17 {
patchfiles-append libavoid-abs-ambiguous.diff
}

This comment has been minimized.

@raimue

raimue Oct 6, 2017

Member

As this is a problem with the source not following C++ rules, the patch should be applied unconditionally.

This comment has been minimized.

@danchr

danchr Oct 7, 2017

Contributor

Fixed.

inkscape: fix build in High Sierra
The current version of Inkscape fails with a build error due to an
ambigious call to abs(). Indeed, i->pathNext->id.objID is of type
unsigned int, for which abs() does not make much sense. Adding a cast
to 'long' fixes it.

The root cause of the bug is likely that the code isn't strictly valid
C++, and so the version of Clang included with Xcode 9 rejects it,
where earlier versions of Clang accepted it. Since this is a build
fix, we don't bump the revision.

(Based on suggestion by Michael Lass)

Fixes: https://trac.macports.org/ticket/54886
@odinsbane

This comment has been minimized.

odinsbane commented Oct 20, 2017

port upgrade outdated was failing on inkscape. I used this patch and the program now compiles and upgrade outdated completes. I hope this pull request can be accepted, or some variant.

@danchr danchr closed this Oct 23, 2017

@danchr danchr deleted the danchr:fix-inkscape-on-high-sierra branch Oct 23, 2017

@danchr

This comment has been minimized.

Contributor

danchr commented Oct 23, 2017

@dbevans didn't respond, so I've pushed the patch as 542661b.

@mclegrand

This comment has been minimized.

mclegrand commented Oct 23, 2017

Hi! I just pushed the fix upstream to .92.x (does not apply to master)
If there are similar problems in master (or .92.x) and you want/need them pushed upstream, feel free to open a merge request on gitlab, they are usually swiftly reviewed/merged (if they don't break the build for other compilers ^^)

@danchr

This comment has been minimized.

Contributor

danchr commented Oct 24, 2017

Thank you! I assume this is the one:

https://gitlab.com/inkscape/inkscape/commit/b2fd37fce9d27998c11c0b40dd10e1624c46b2d4

Normally, I'd replace my patch with the upstream one, but since they're the same (and Inkscape isn't exactly a small application) I'll skip it this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment