Skip to content

Conversation

@ginggs
Copy link
Contributor

@ginggs ginggs commented Oct 12, 2015

Originally posted by @pao in #10791

[pao: fix my own username :D]

@nalimilan
Copy link
Member

The original piece of code looks strange to me. Couldn't there simply be a else clause setting ISX86:=0 for all other architectures? Looks like we only set BINARY for x86 anyway, and we know all possible ARCH values for this platform.

@ginggs
Copy link
Contributor Author

ginggs commented Oct 12, 2015

@nalimilan: thanks! updated.

@nalimilan
Copy link
Member

@ginggs Thanks, but I should have made it clearer that my comment was addressed to other developers to get their opinion about this. I hope they don't prove me wrong and that you don't have to go back to your original patch...

@pao pao added building Build system, or building Julia or its dependencies system:arm ARMv7 and AArch64 labels Oct 12, 2015
@ginggs
Copy link
Contributor Author

ginggs commented Oct 12, 2015

@nalimilan No worries, that was how I understood your comment. I figured I'd just provide both options, for comparison.

@pao Sorry for mangling your username!

@pao
Copy link
Member

pao commented Oct 12, 2015

No problem! I probably should have submitted this; just didn't think about it. Thanks!

@nalimilan
Copy link
Member

@tkelman @ViralBShah Is this OK to merge? I've just bumped into this problem, and indeed raising an error without even allowing the caller to specify the word size by hand is quite inconvenient.

@tkelman
Copy link
Contributor

tkelman commented Oct 17, 2015

This is probably okay, but I do wonder what would happen with random other exotic arches that people may try to build Julia on.

@nalimilan
Copy link
Member

It will probably fail somewhere in the build. :-)

The point is to make it possible to continue with the build, trying several build flags until it (possibly) works.

@ViralBShah
Copy link
Member

This is good to merge to solve the immediate problem. Should ideally squash these commits though.

This allows the build to proceed for AArch64 (mentioned in #10791) and others.
@ginggs
Copy link
Contributor Author

ginggs commented Oct 18, 2015

Squashed. Thanks for considering!

ViralBShah added a commit that referenced this pull request Oct 18, 2015
Define AArch64 as "not x86"
@ViralBShah ViralBShah merged commit 6f0b15b into JuliaLang:master Oct 18, 2015
@ViralBShah
Copy link
Member

I guess BINARY is not strictly required here, but may be required eventually to get 64-bit BLAS going and such.

@ginggs ginggs deleted the patch-1 branch October 18, 2015 13:50
@ginggs
Copy link
Contributor Author

ginggs commented Oct 18, 2015

Could this be backported to the 0.4 series please?

@StefanKarpinski
Copy link
Member

If it doesn't cause any problems on master for a bit this should be safe.

@IainNZ
Copy link
Member

IainNZ commented Oct 18, 2015

@tkelman I labelled it so it can at least be considered, not a vouch of confidence for backportability

@ViralBShah
Copy link
Member

This should be safe for a backport.

tkelman pushed a commit that referenced this pull request Oct 31, 2015
This allows the build to proceed for AArch64 (mentioned in #10791) and others.

(cherry picked from commit f244fb6)
ref #13558
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

building Build system, or building Julia or its dependencies system:arm ARMv7 and AArch64

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants