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

Fix Issue 21222 - Add windows 64 bit version of rdmd #484

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Jul 16, 2021

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Auto-close Bugzilla Severity Description
21222 major Add windows 64 bit version of rdmd

@RazvanN7 RazvanN7 force-pushed the Winpath branch 3 times, most recently from 68484a0 to 9de817d Compare July 16, 2021 08:25
@@ -1197,7 +1197,7 @@ write_env_vars() {
echo "set _OLD_D_PATH=%PATH%"
echo "set PATH=${DUB_BIN_PATH:+$(cygpath -w "${DUB_BIN_PATH}");}${winpath};%PATH%"
if [[ $PROCESSOR_ARCHITECTURE != x86 ]]; then
echo "set PATH=${winpath}64;%PATH%"
echo "set PATH=${winpath}64;%${winpath};PATH%"
Copy link
Member

Choose a reason for hiding this comment

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

But, isn't this redundant? The line prepended a value to PATH, which already contains winpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, you are right. Then why wasn't this [1] working? (I don't have a windows machine, I'm just shooting in the dark here)

[1] https://forum.dlang.org/post/[email protected]

Copy link
Member

Choose a reason for hiding this comment

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

I think the poster there was using the .exe installers and not this script.

@@ -295,6 +295,7 @@ SectionGroup /e "D2"

Section "Add to PATH" AddD2ToPath
${EnvVarUpdate} $0 "PATH" "A" "HKLM" "$INSTDIR\dmd2\windows\bin"
${EnvVarUpdate} $0 "PATH" "A" "HKLM" "$INSTDIR\dmd2\windows\bin64"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CyberShadow This was the only place where the path wasn't set to bin64 although I haven't found a way to do it depending on the arch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I have no idea why this [1] isn't working. I haven't done windows stuff since forever.

[1] https://github.com/dlang/installer/pull/484/files#diff-179a67f45668adad5ec8526432aeda299d6c08b4df2bb5349f37489fda1674d4R277

@CyberShadow
Copy link
Member

@rainers is the expert - any chance you could have a look?

@rainers
Copy link
Member

rainers commented Jul 16, 2021

There are no executables in bin64 in the installer artifact other than dmd.exe, dub.exe and lld-link.exe. It might be useful for rdmd.exe to pick up the 64-bit compiler, but other tools like ddemangle.exe or dustmite.exe don't need much memory themselves.

To have the 32-bit tools available on 64-bit, too, dmd2vars64.bat adds both bin folders to PATH, see https://github.com/dlang/installer/blob/master/windows/d2-installer.nsi#L276. If done through a registry entry, the same should probably happen.

@RazvanN7
Copy link
Contributor Author

@rainers I'm guessing something went wrong with [1] that is causing [2]. The only difference that I could spot with with regards to setting the PATH is in the registry entry setting (missing for the 64-bit directory).

How should we proceed?

[1] https://github.com/dlang/installer/blob/master/windows/d2-installer.nsi#L276
[2] https://forum.dlang.org/post/[email protected]

@rainers
Copy link
Member

rainers commented Jul 31, 2021

I'm not sure what the problem actually is: A 64-bit rdmd doesn't really do anything different than a 32-bit version. If also placed in the bin64 folder, it will use the 64-bit dmd.exe, but that will not do anything different than the 32-bit version of dmd (especially not build 64-bit executables).
The only reason to use the 64-bit dmd is when you need a lot of memory to compile, but that is unlikely to be the case if you use script like programs with rdmd.

If you want to actually want rdmd in the bin64 folder, you need to update the release build program around here: https://github.com/dlang/installer/blob/master/create_dmd_release/create_dmd_release.d#L595

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Aug 1, 2021

As far as I understand, this user [1] could not use rdmd because because path wasn't set appropriately: "I added this to my path and it runs, but are there any implications for building 64 bit apps". I am interpreting this as: rdmd was not found when for the 64-bit installation. Looking at the nsi script, it seems that $PATH is indeed set to bin32 and bin64 both on 64-bit systems, but then how do we explain the scenario encountered by the above mentioned user?

[1] https://forum.dlang.org/post/[email protected]

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

Successfully merging this pull request may close these issues.

4 participants