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

appveyor: also test Visual Studio 2017 and investigate NMake generator as we support them #262

Merged
merged 5 commits into from Jan 9, 2020

Conversation

@illwieckz
Copy link
Member

illwieckz commented Jan 5, 2020

README says the minimum supported VS is VS2017 so it's better to test it

Unvanquished uses NMake Makefiles generator so it's better to test it

@illwieckz illwieckz force-pushed the illwieckz:appveyor branch 13 times, most recently from 1dd6889 to f21f58c Jan 5, 2020
@illwieckz illwieckz changed the title appveyor: also test Visual Studio 2017 as we support it appveyor: also test Visual Studio 2017 and NMake generator as we support it Jan 6, 2020
@illwieckz illwieckz force-pushed the illwieckz:appveyor branch 6 times, most recently from f0d2779 to cb341f9 Jan 6, 2020
@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 6, 2020

OK, this looks OK to me now.

I don't have solution to build NMake 32-bit build, I don't know why Unvanquished manages to do it.

@illwieckz illwieckz force-pushed the illwieckz:appveyor branch from cb341f9 to 5a1c5cf Jan 6, 2020
@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 6, 2020

The bad thing is that because of testing those various build scenarii, the appveyor run now lasts 20min20 (example).

Appveyor is slow, on the contrary Travis best run is around 2min30 and worst one is around 4min20, for a total of 5min20 (example).

The reason is that Appveyor does not build those tests in parallel, unlike Travis, and each run lasts about 5 minutes.
Also, unlike Travis, an Appveyor run is slow by itself. I noticed only 2 CPUs are allocated.

We now test:

  • VS2019 amd64
  • VS2019 i386
  • VS2019 Nmake amd64
  • VS2017 amd64

If we do that:

  • VS2019 amd64
  • VS2019 Nmake amd64
  • VS2017 i386

We would save 1 run, reducing the total run time to 15min instead of 20min.

@illwieckz illwieckz changed the title appveyor: also test Visual Studio 2017 and NMake generator as we support it appveyor: also test Visual Studio 2017 and NMake generator as we support them Jan 6, 2020
@illwieckz illwieckz added the Build label Jan 6, 2020
@slipher

This comment has been minimized.

Copy link
Member

slipher commented Jan 8, 2020

There's no real reason to care about the NMake Makefiles generator. I always figured that it must be needed because of the Visual Studio generator being only usable with the IDE, but since you can apparently use the VS generator on Appveyor, it seems better to just do that.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 8, 2020

OKOK. If you don't mind, I would add a commit to disable NMake and not squash them so we keep knowledge in git history. Getting it working was not that easy. Losing the information would be a shame.

What about testing 64-bit on VS2019 and 32-bit on VS2017? it would reduce the build time to 10min.
That would not be a complete test, but eh… 32-bit people are likely to run old OS hence old VS…

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 8, 2020

Well, that would be a shame to discover the day of the release we fail to build 32-bit on top-notch setup because of a silent bug…

@slipher

This comment has been minimized.

Copy link
Member

slipher commented Jan 8, 2020

What about testing 64-bit on VS2019 and 32-bit on VS2017? it would reduce the build time to 10min.

Yes, I think this is good enough. Rather than filling every cell of the build matrix, so to speak, let's try to get at least one entry in each row/column.

Well, that would be a shame to discover the day of the release we fail to build 32-bit on top-notch setup because of a silent bug…

Well, we've always used Mingw for releases anyway.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 8, 2020

right

illwieckz added 5 commits Jan 5, 2020
README says the minimum supported VS is VS2017
so it's better to test it
cmake -H and -B were undocumented
CMake 3.13 introduced official -S to replace undocumented -H
CMake 3.13 started to officially support -B
Later CMake versions may reuse -H as an alias to --help

Appveyor Visual Studio 2015, 2017 and 2019 provide CMake 3.15.5
- NMake: keep knowledge in git history, save build time
  see 96d5c1f
- VS 2012/19, 32/64-Bit: let's try to get at least one
  entry in each row/column, we've always used Mingw for
  releases anyway
@illwieckz illwieckz force-pushed the illwieckz:appveyor branch from 5a1c5cf to 9f2554e Jan 8, 2020
@illwieckz illwieckz changed the title appveyor: also test Visual Studio 2017 and NMake generator as we support them appveyor: also test Visual Studio 2017 and investigate NMake generator as we support them Jan 8, 2020
@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 9, 2020

so I merged with VS2017-32 and VS2019-64 being built only

note that there is there a good example of MSYS2-in-Appveyor implementation: mypaint/libmypaint@553f175

and we may also implement appveyor conditional build: https://www.appveyor.com/blog/2018/09/11/how-to-skip-build-matrix-jobs-conditionally/

@illwieckz illwieckz merged commit 9f2554e into DaemonEngine:master Jan 9, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@illwieckz illwieckz deleted the illwieckz:appveyor branch Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.