Add win64 build and test or something #7214

dmdw64 · 2017-10-12T23:39:29Z

No description provided.

dlang-bot · 2017-10-12T23:39:30Z

Thanks for your pull request, @dmdw64! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

wilzbach · 2017-12-10T07:02:01Z

appveyor.yml

+  - cd c:/projects/
+  - call "c:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat" amd64
+  - bash --version
+  - appveyor DownloadFile "https://ftp.gnu.org/gnu/make/make-4.2.tar.gz" -FileName make.tar.gz


While I would love to use GNUmake on Windows, the sad reality is that DMD ships with DM make and thus that's the only make binary we can expect people to have on Windows. @CyberShadow and I have tried to push for changing the status quo in the past, but so far without any real progress :/

The test suite actually uses GNU make on Windows, too. You even need a unix like environment to run the *.sh tests.

@wilzbach

As mentioned, it's required to run the tests. I'd be fine with just using GNU make for all the make scripts. It's not fun using two different make systems who's executables are both named "make". I'd much rather have one make system, then GNU make can be provided with the releases instead of DM's make.

Fixing the make situation would also be nice. Windows has the most amount of make configurations but the least amount of test servers. It's not ideal at all, this pull request was to at the very least get win64.mak tested as it isn't being run as part of any tests at all. This PR was sitting here for ~2 months before it got its first comment..

We already have a go for GNU make on Windows @wilzbach, just needs someone to implement the transition.

How about caching that installation?
https://www.appveyor.com/docs/build-cache/

wilzbach · 2017-12-17T10:26:22Z

@dmdw64 I just enabled the AppVeyor hook again and closed/reopened your PR to trigger a build.

wilzbach

Fixing the make situation would also be nice. Windows has the most amount of make configurations but the least amount of test servers. It's not ideal at all,

Agreed.

this pull request was to at the very least get win64.mak tested as it isn't being run as part of any tests at all. This PR was sitting here for ~2 months before it got its first comment..

I'm really sorry about this. Imho we should move on with this as an initial version as soon as the tests on AppVeyor pass. What do other people think?
CC @CyberShadow @ZombineDev @MartinNowak

wilzbach · 2017-12-17T10:29:01Z

appveyor.sh

+    dmd --version
+fi
+
+if [ $APPVEYOR_REPO_TAG == true ]; then


We need to check whether the upstream branch (e.g. stable) exists on these repos too.
See e.g.:

dmd/circleci.sh

Lines 84 to 92 in d28274e

for proj in druntime phobos; do

if [ $base_branch != master ] && [ $base_branch != stable ] &&

! git ls-remote --exit-code --heads https://github.com/dlang/$proj.git $base_branch > /dev/null; then

# use master as fallback for other repos to test feature branches

clone https://github.com/dlang/$proj.git ../$proj master --depth 1

else

clone https://github.com/dlang/$proj.git ../$proj $base_branch --depth 1

fi

done

and

dmd/travis.sh

Lines 94 to 102 in d28274e

for proj in druntime phobos; do

if [ $TRAVIS_BRANCH != master ] && [ $TRAVIS_BRANCH != stable ] &&

! git ls-remote --exit-code --heads https://github.com/dlang/$proj.git $TRAVIS_BRANCH > /dev/null; then

# use master as fallback for other repos to test feature branches

clone https://github.com/dlang/$proj.git ../$proj master

else

clone https://github.com/dlang/$proj.git ../$proj $TRAVIS_BRANCH

fi

done

wilzbach · 2017-12-17T10:30:16Z

appveyor.sh

+    git clone --branch master https://github.com/dlang/druntime
+    git clone --branch master https://github.com/dlang/phobos
+fi
+


Not sure whether CircleCi supports it, but otherwise we need to manually merge the PR into master, see e.g.:

dmd/circleci.sh

Lines 73 to 82 in d28274e

# merge testee PR with base branch (master) before testing

if [ -n "${CIRCLE_PR_NUMBER:-}" ]; then

local head=$(git rev-parse HEAD)

git fetch https://github.com/dlang/$CIRCLE_PROJECT_REPONAME.git $base_branch

git checkout -f FETCH_HEAD

local base=$(git rev-parse HEAD)

git config user.name 'CI'

git config user.email '<>'

git merge -m "Merge $head into $base" $head

fi

I think it does do the merge first, at least by how it grabs the commit it looks like it does.

dmdw64 · 2017-12-17T18:02:10Z

Think you can enable appveyor again? It doesn't seem to be detecting the changes.

Might have to use an older VS, I think I remember not being able to get it work with VS2015 for whatever reason. DMD would always crash when trying to build but would work with the older VS2010.

wilzbach · 2017-12-17T19:21:27Z

Think you can enable appveyor again? It doesn't seem to be detecting the changes.

Sorry. I won't disable it again.

Might have to use an older VS, I think I remember not being able to get it work with VS2015 for whatever reason. DMD would always crash when trying to build but would work with the older VS2010.

That sounds really terribly. I presume @rainers could help here to get this PR to the finish line?

wilzbach · 2017-12-17T19:30:50Z

/c/projects/dmd/appveyor.sh: line 25: clone: command not found

clone is defined in the CI scripts I referenced:

dmd/circleci.sh

Lines 17 to 31 in d28274e

    
           clone() { 
        
               local url="$1" 
        
               local path="$2" 
        
               local branch="$3" 
        
               for i in {0..4}; do 
        
                   if git clone --branch "$branch" "$url" "$path" "${@:4}"; then 
        
                       break 
        
                   elif [ $i -lt 4 ]; then 
        
                       sleep $((1 << $i)) 
        
                   else 
        
                       echo "Failed to clone: ${url}" 
        
                       exit 1 
        
                   fi 
        
               done 
        
           }

rainers · 2017-12-17T22:48:30Z

That sounds really terribly. I presume @rainers could help here to get this PR to the finish line?

dmd is currently broken when building COFF due to #7455.

PetarKirov · 2017-12-22T14:03:59Z

Let's try restarting the appveyor build.

wilzbach · 2017-12-22T14:06:37Z

Let's try restarting the appveyor build.

Rebased to master to incorporate the news changes (that's merging with master should be done as part of the script).

PetarKirov · 2017-12-22T14:49:05Z

Finally! Thanks to @dmdw64 and @wilzbach's efforts the AppVeyor build is looking like it's going to pass for the first time. At this rate the log output in AppVeyor behaves like a browser benchmark 😄 (Of all browsers on my computer only Microsoft Edge is able to smoothly display the log, coincidence?!)

wilzbach · 2017-12-22T14:57:47Z

Well it was @rainers who fixed DMD, which clearly shows why there is a need for such an CI. Luckily the respective PR was merged yesterday and is available via dmd-nightly. :)

MartinNowak

Looks mostly good.

MartinNowak · 2017-12-27T20:43:32Z

appveyor.yml

+      C_COMPILER:          MSVC
+
+artifacts:
+  - path: src/dmd.exe


Why would you want to provide artifacts?
We already have http://nightlies.dlang.org/ for nightlies.

MartinNowak · 2017-12-27T20:46:18Z

appveyor.yml

+  - cd c:/projects/
+  - call "c:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat" amd64
+  - bash --version
+  - appveyor DownloadFile "https://ftp.gnu.org/gnu/make/make-4.2.tar.gz" -FileName make.tar.gz


How about caching that installation?
https://www.appveyor.com/docs/build-cache/

MartinNowak · 2017-12-27T20:48:58Z

appveyor.sh

@@ -0,0 +1,64 @@
+#!/bin/sh


Why should this be a bash/sh file, makes it more complex to reproduce, and there are already batch and powershell. Also this script does very little, so it hardly should a problem to reproduce it in PowerShell.

Need to use bash in order to run the tests for dmd, so there's no avoiding it completely. Can make it all a powershell script except for the part that's needed to run the tests.

I agree, it's not a big deal to use bash here. It is not an additional dependency as the tests require it anyway.

rainers · 2017-12-27T22:18:53Z

It takes about 20 minute, while LDC builds the tests in about 8 minutes.

LDC builds are faster because not all combinations of command line flags are tested, just the quick tests. With -j1 needed 40 minutes, we are back to 20 minutes with -j2.

dmdw64 · 2017-12-28T02:44:07Z

@MartinNowak
Well this is also part of why I used bash, it's much more CI friendly and the logs it produces are much nicer. I'll tinker around with it some more later maybe but I think it would just be better to use bash. You also can't really run the scripts locally anyways as it uses appveyor specific commands/variables.

Moved here: https://github.com/dmdw64/dmd/blob/ps1/appveyor.ps1
If needed.

dmdw64 · 2017-12-28T03:59:55Z

@rainers
I don't know if we should test 32mscoff? Having 64-bit binary is of benefit, but if dmd wants to use optlink for their 32-bit builds then I don't see sense in having 2 of the same.

Should definitely make sure the VisualD project file builds. Don't know how you would go about installing VisualD into appveyor.

I'll add the unittests for phobos/druntime when I get a chance tomorrow.

rainers · 2017-12-28T09:09:53Z

I don't know if we should test 32mscoff? Having 64-bit binary is of benefit, but if dmd wants to use optlink for their 32-bit builds then I don't see sense in having 2 of the same.

The auto tester is testing the generation of 64-bit COFF and 32-bit OMF. 32-bit COFF is not covoered yet.

Should definitely make sure the VisualD project file builds. Don't know how you would go about installing VisualD into appveyor.

I think this can go into a new PR. I can add that integration, I've done that for Visual D, too.

I'll add the unittests for phobos/druntime when I get a chance tomorrow.

I think this can also wait for another PR.

wilzbach

I think this can go into a new PR.

Yes, move in small, incremental steps, but succeed :)
Imho this is more than enough for an initial version. I prefer Bash vastly over Powershell too - we need bash anyways and Bash is something we all are familiar with and can run locally as well.
Also this would allow use to share code between Travis, CircleCi and AppVeyor.

Thanks again to everyone for pushing and working on this!

wilzbach · 2017-12-28T09:26:02Z

appveyor.yml

+
+build_script:
+  - cd c:/projects/
+  - call "c:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat" amd64


If I understand @rainers work correctly, this won't be necessary anymore in a foreseeable future

wilzbach · 2017-12-28T09:27:54Z

appveyor.yml

+  matrix:
+    - ARCH:                x64
+      D_COMPILER:          dmd
+      D_VERSION:           2.077.1


We shouldn't forget to change this to 2.078.0 once released and remove the hard-coded dmd-nightly.

wilzbach · 2017-12-28T09:33:50Z

The merging with master

This is still missing too, but it's not really blocking. I am more than happy to quickly add it afterwards.

Anyone against squashing our combined work into one commit?

rainers · 2017-12-28T10:21:36Z

This is still missing too

Isn't this done automatically, see for example https://ci.appveyor.com/project/greenify/dmd/build/1.0.373#L4

Anyone against squashing our combined work into one commit?

Fine with me.

wilzbach · 2017-12-28T10:35:13Z

Isn't this done automatically, see for example

Oh yes. I should take more care when looking at stuff from my phone. Sorry.

Anyone against squashing our combined work into one commit?
Fine with me.

OK done.

MartinNowak · 2017-12-28T15:10:40Z

appveyor.yml

+
+environment:
+  matrix:
+    - ARCH:                x64


ARCH isn't used below.

dmdw64 force-pushed the master branch 2 times, most recently from 8fe87a2 to be1c195 Compare October 16, 2017 20:34

rainers mentioned this pull request Dec 8, 2017

[WIP] Add AppVeyor config to test Visual Studio builds #7412

Closed

wilzbach reviewed Dec 10, 2017

View reviewed changes

wilzbach mentioned this pull request Dec 11, 2017

Cleanup DMD as a library build files #7418

Merged

wilzbach closed this Dec 17, 2017

wilzbach reopened this Dec 17, 2017

wilzbach approved these changes Dec 17, 2017

View reviewed changes

dmdw64 force-pushed the master branch from be1c195 to 8481655 Compare December 17, 2017 17:46

dmdw64 closed this Dec 17, 2017

dmdw64 reopened this Dec 17, 2017

wilzbach closed this Dec 17, 2017

wilzbach reopened this Dec 17, 2017

dmdw64 force-pushed the master branch from 8481655 to adb472f Compare December 17, 2017 21:27

rainers mentioned this pull request Dec 22, 2017

fix building for coverage analysis on windows #7491

Merged

PetarKirov closed this Dec 22, 2017

PetarKirov reopened this Dec 22, 2017

wilzbach force-pushed the master branch 3 times, most recently from 5f82c9b to 19ce119 Compare December 22, 2017 14:40

MartinNowak reviewed Dec 27, 2017

View reviewed changes

dmdw64 force-pushed the master branch 4 times, most recently from e7f44ec to 9bdd11c Compare December 28, 2017 01:58

dmdw64 closed this Dec 28, 2017

dmdw64 reopened this Dec 28, 2017

dmdw64 force-pushed the master branch from 9bdd11c to 586a756 Compare December 28, 2017 02:29

dmdw64 closed this Dec 28, 2017

dmdw64 reopened this Dec 28, 2017

dmdw64 force-pushed the master branch from 586a756 to 604b0dd Compare December 28, 2017 02:33

dmdw64 force-pushed the master branch 7 times, most recently from 64f24d0 to 919f8d7 Compare December 28, 2017 03:55

wilzbach approved these changes Dec 28, 2017

View reviewed changes

Add win64 build and test.

d2f4e51

wilzbach force-pushed the master branch from 919f8d7 to d2f4e51 Compare December 28, 2017 10:31

MartinNowak reviewed Dec 28, 2017

View reviewed changes

appveyor.yml

environment:

matrix:

- ARCH: x64

Copy link

Member

MartinNowak Dec 28, 2017

Choose a reason for hiding this comment

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

ARCH isn't used below.

MartinNowak merged commit b8267bd into dlang:master Dec 28, 2017

	for proj in druntime phobos; do
	if [ $base_branch != master ] && [ $base_branch != stable ] &&
	! git ls-remote --exit-code --heads https://github.com/dlang/$proj.git $base_branch > /dev/null; then
	# use master as fallback for other repos to test feature branches
	clone https://github.com/dlang/$proj.git ../$proj master --depth 1
	else
	clone https://github.com/dlang/$proj.git ../$proj $base_branch --depth 1
	fi
	done

	for proj in druntime phobos; do
	if [ $TRAVIS_BRANCH != master ] && [ $TRAVIS_BRANCH != stable ] &&
	! git ls-remote --exit-code --heads https://github.com/dlang/$proj.git $TRAVIS_BRANCH > /dev/null; then
	# use master as fallback for other repos to test feature branches
	clone https://github.com/dlang/$proj.git ../$proj master
	else
	clone https://github.com/dlang/$proj.git ../$proj $TRAVIS_BRANCH
	fi
	done

	# merge testee PR with base branch (master) before testing
	if [ -n "${CIRCLE_PR_NUMBER:-}" ]; then
	local head=$(git rev-parse HEAD)
	git fetch https://github.com/dlang/$CIRCLE_PROJECT_REPONAME.git $base_branch
	git checkout -f FETCH_HEAD
	local base=$(git rev-parse HEAD)
	git config user.name 'CI'
	git config user.email '<>'
	git merge -m "Merge $head into $base" $head
	fi

Add win64 build and test or something #7214

Add win64 build and test or something #7214

Conversation

dmdw64 commented Oct 12, 2017

dlang-bot commented Oct 12, 2017

Bugzilla references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmdw64 Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Dec 17, 2017

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmdw64 Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

dmdw64 commented Dec 17, 2017

wilzbach commented Dec 17, 2017

wilzbach commented Dec 17, 2017

rainers commented Dec 17, 2017

PetarKirov commented Dec 22, 2017

wilzbach commented Dec 22, 2017

PetarKirov commented Dec 22, 2017 • edited Loading

wilzbach commented Dec 22, 2017

MartinNowak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Dec 27, 2017

dmdw64 commented Dec 28, 2017 • edited Loading

dmdw64 commented Dec 28, 2017 • edited Loading

rainers commented Dec 28, 2017

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Dec 28, 2017

rainers commented Dec 28, 2017

wilzbach commented Dec 28, 2017

Choose a reason for hiding this comment

dmdw64 Dec 16, 2017 •

edited

Loading

dmdw64 Dec 17, 2017 •

edited

Loading

PetarKirov commented Dec 22, 2017 •

edited

Loading

dmdw64 commented Dec 28, 2017 •

edited

Loading

dmdw64 commented Dec 28, 2017 •

edited

Loading