-
-
Notifications
You must be signed in to change notification settings - Fork 598
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
build.d: Use alternative host model detection for Windows #10491
Conversation
Thanks for your pull request and interest in making D better, @GoaLitiuM! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#10491" |
int is64; | ||
if (IsWow64Process(GetCurrentProcess(), &is64)) | ||
return is64 ? "64" : "32"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps else static assert(false);
for prudence / clarity, though the current code will have the same effect due to not returning a value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean else assert(false);
? Otherwise this would stop compiling on non-Windows systems, this is a runtime check and not compile-time check after all.
{ | ||
version (Windows) | ||
{ | ||
import core.sys.windows.winbase; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add:
version (D_LP64)
return "64";
else
{
...
so that we don't even call IsWow64Process
on 64 bits, we already know we're 64-bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even version (Win64) return "64";
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be a runtime check so that you can build a 64-bit executable using a 32-bit host compiler(?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. I suppose you can use a 32-bit compiler to compile build.d and then use it to build a 64-bit compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't a 32-bit dmd detect that it's on a 64-bit Windows machine and set the version appropriately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import std.stdio;
void main()
{
version (Win32)
writeln("win32");
version (Win64)
writeln("win64");
version (D_LP64)
writeln("D_LP64");
}
On my 64-bit windows machine, running a 32-bit dmd compiler, I get this output:
win32
If I add -m64
, then I get this:
win64
D_LP64
So I guess the question is, do we want the default to match the model that the developer used to compile build.d
, or do we want the default to always use 64 on 64-bit machines and 32 otherwise? In the end, the default doesn't matter to much, it's just the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to use version
here for defaults, why not extend this to other platforms as well and just use this:
auto detectModel()
{
version (D_LP64) return "64";
else return "32";
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is becoming a bit of a Chinese whispers situation.
- It is kind of weird how a compile-time condition (OS
version
) is translated into a runtime string value, which is then used to make decisions. I'm not sure if this is an artifact from a literal translation from makefiles, or some kind of future-proofing for cross-compilation. - My understanding that for Windows we want to target the same bitness as the OS, not compiled executable, so this needs to be a runtime check.
- The old method, calling
wmic.exe
, was needlessly expensive (running a separate program) and fragile (broke Digger, and output depends on the current locale). We should be using Windows APIs instead. - Because D and Windows come in only 32-bit and 64-bit flavors, and because you can't run 64-bit executables on 32-bit Windows, I suggested checking
D_LP64
as an optimization: if the current executable is 64-bit, then we can infer that the OS must also be 64-bit, so go ahead and target 64-bit without checking the OS bitness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyberShadow I see now, a clever optimization. If we were to use that, I would probably suggest a comment like this:
// If build.d is compiled as 64-bit, then we MUST be on a 64-bit system. The inverse is not guaranteed.
version (D_LP64)
return "64";
else
{
Altered the changes based on feedback. |
WMIC output seems to vary based on the host locale, and we don't want to patch the output detection to include all different localizations which are hard to test.
Tests were failing, removed the |
WMIC output seems to vary based on the host locale, and we don't want to patch the output detection to include all different localizations which are hard to test.