enable vcpkg for all platforms on CI #242

Arcnor · 2024-04-30T06:54:26Z

Pull Request Type

Description

Enable vcpkg on all platforms, include & link SDL2 for all of them as well

Related Issues

Relates to #241

Screenshots (if applicable)

Checklist

I have tested my changes locally and verified that they work as intended.
I have documented any new or modified functionality.
I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Additional Comments

Lgt2x

Update README instructions to use VCPKG on Linux ? Good job otherwise

Lgt2x · 2024-04-30T18:59:07Z

.github/workflows/build.yml

      - name: Install Linux dependencies
        if: ${{ matrix.os.preset == 'linux' }}
        run: |
          sudo apt update
          sudo apt install -y --no-install-recommends \
-            ninja-build cmake g++ libgtest-dev libsdl1.2-dev libsdl-image1.2-dev libncurses-dev zlib1g-dev
+            ninja-build cmake g++ libgtest-dev libsdl1.2-dev libsdl-image1.2-dev libncurses-dev zlib1g-dev \


do we need all those packages when using VCPKG ?

We do not, but if I remove them the current build will fail until we actually remove SDL1 from the source :)

Please mark those concerned for deletion (ie on a separate line with a comment)

Descent3/CMakeLists.txt

vcpkg.json

winterheart

I don't mind against using vcpkg on Linux and macOS, but most likely Linux distributions will force using system libraries like SDL2 or spdlog from their repositories, so we must provide them ability to do so (again, proposed changes allows that, so OK from me).

Arcnor · 2024-04-30T22:12:15Z

Update README instructions to use VCPKG on Linux ? Good job otherwise

Was hoping you could help me with that :D. I had a change where I basically added the same line as on Windows, but it seems macOS and Linux have very specific instructions, so in the same vein maybe we can provide exact instructions. If you can try to install vcpkg and tell me the proper paths I can add them.

Arcnor · 2024-04-30T22:31:44Z

I don't mind against using vcpkg on Linux and macOS, but most likely Linux distributions will force using system libraries like SDL2 or spdlog from their repositories, so we must provide them ability to do so (again, proposed changes allows that, so OK from me).

Well, this is just for building, I think people can use whatever they want for running, but specifying what they need for building is something I think we can do, and in this case it can be "vcpkg is required". That said, I can wait until Ryan's PR is merged, then I add all of this on top of his stuff, and basically fallback to the system one if it cannot find the CONFIG one.

Lgt2x · 2024-05-01T18:51:37Z

Update README instructions to use VCPKG on Linux ? Good job otherwise

Was hoping you could help me with that :D. I had a change where I basically added the same line as on Windows, but it seems macOS and Linux have very specific instructions, so in the same vein maybe we can provide exact instructions. If you can try to install vcpkg and tell me the proper paths I can add them.

On a fresh ubuntu:latest docker image, I can build using the following commands (first line is new VCPKG requirements):

apt install -y git cmake g++ curl zip unzip tar ninja-build pkg-config python3 python3-distutils-extra python3-jinja2 autoconf autopoint libtool build-essential gperf 
apt install -y --no-install-recommends ninja-build cmake g++ libsdl1.2-dev libsdl-image1.2-dev libncurses-dev  zlib1g-dev libspdlog-dev
git clone https://github.com/microsoft/vcpkg
./vcpkg/bootstrap-vcpkg.sh
VCPKG_ROOT=./vcpkg cmake --preset linux
cmake --build --preset linux --config Debug

However, I'm not sure about recommending this approach to anyone wanting to build on Linux. It has much more requirements in order to bulid the VCPKG packages, and the build is way longer. In addition, VCPKG errors can be obscure and harder to debug (it's not just written plainly "Missing package X")

Arcnor · 2024-05-03T19:57:39Z

Update README instructions to use VCPKG on Linux ? Good job otherwise

Was hoping you could help me with that :D. I had a change where I basically added the same line as on Windows, but it seems macOS and Linux have very specific instructions, so in the same vein maybe we can provide exact instructions. If you can try to install vcpkg and tell me the proper paths I can add them.

On a fresh ubuntu:latest docker image, I can build using the following commands (first line is new VCPKG requirements):
apt install -y git cmake g++ curl zip unzip tar ninja-build pkg-config python3 python3-distutils-extra python3-jinja2 autoconf autopoint libtool build-essential gperf 
apt install -y --no-install-recommends ninja-build cmake g++ libsdl1.2-dev libsdl-image1.2-dev libncurses-dev libxext6:i386 zlib1g-dev spdlog-dev
git clone https://github.com/microsoft/vcpkg
./vcpkg/bootstrap-vcpkg.sh
VCPKG_ROOT=./vcpkg cmake --preset linux
cmake --build --preset linux --config Debug
However, I'm not sure about recommending this approach to anyone wanting to build on Linux. It has much more requirements in order to bulid the VCPKG packages, and the build is way longer. In addition, VCPKG errors can be obscure and harder to debug (it's not just written plainly "Missing package X")

To be honest, those packages you'll need anyway for any kind of development, and the build is longer only the very first time, which is not a useful metric when we have cache IMHO. On the CI agents I had to add none, but I'm not sure that's a useful metric either.

About the errors, I'll need an example to know why you think they are hard to understand, but vcpkg only calls CMake under the hood, and it clearly states where the log with the errors is.

Finally, if you think this is not useful let's just close and maybe remove vcpkg itself like I mentioned on Discord, we just need consensus on that.

This incorporates #242 and #276, and resolves #258

Arcnor · 2024-05-04T07:01:20Z

@JeodC what you just did makes no sense. You don't "supersede" by manually copying these changes into new commits you just create, you create your changes on top (or rebase) if you're going to add them. I've reopened this and closed yours.

Jayman2000 · 2024-05-04T19:26:27Z

I've reopened this and closed yours.

A similar thing happened to #281. Do you think that that PR should be reopened? For context, there’s two PRs that would fix #276, but they’re both closed at the moment.

JeodC · 2024-05-04T20:13:03Z

I've reopened this and closed yours.

A similar thing happened to #281. Do you think that that PR should be reopened? For context, there’s two PRs that would fix #276, but they’re both closed at the moment.

I did not expect #284 to be abruptly closed as it was. I tried to reopen it but the branch it originated from was force-pushed or recreated so the button is blank.

Jayman2000 · 2024-05-04T21:09:30Z

I tried to reopen it but the branch it originated from was force-pushed or recreated so the button is blank.

OK. I created a new PR that uses the same branch.

Lgt2x · 2024-05-06T08:15:28Z

Update README instructions to use VCPKG on Linux ? Good job otherwise

Was hoping you could help me with that :D. I had a change where I basically added the same line as on Windows, but it seems macOS and Linux have very specific instructions, so in the same vein maybe we can provide exact instructions. If you can try to install vcpkg and tell me the proper paths I can add them.

On a fresh ubuntu:latest docker image, I can build using the following commands (first line is new VCPKG requirements):
apt install -y git cmake g++ curl zip unzip tar ninja-build pkg-config python3 python3-distutils-extra python3-jinja2 autoconf autopoint libtool build-essential gperf 
apt install -y --no-install-recommends ninja-build cmake g++ libsdl1.2-dev libsdl-image1.2-dev libncurses-dev libxext6:i386 zlib1g-dev spdlog-dev
git clone https://github.com/microsoft/vcpkg
./vcpkg/bootstrap-vcpkg.sh
VCPKG_ROOT=./vcpkg cmake --preset linux
cmake --build --preset linux --config Debug
However, I'm not sure about recommending this approach to anyone wanting to build on Linux. It has much more requirements in order to bulid the VCPKG packages, and the build is way longer. In addition, VCPKG errors can be obscure and harder to debug (it's not just written plainly "Missing package X")
To be honest, those packages you'll need anyway for any kind of development, and the build is longer only the very first time, which is not a useful metric when we have cache IMHO. On the CI agents I had to add none, but I'm not sure that's a useful metric either.

True, only the first build is slower.

About the errors, I'll need an example to know why you think they are hard to understand, but vcpkg only calls CMake under the hood, and it clearly states where the log with the errors is.

I lost my terminal session since then, but the 2 layers VCPKG, and CMake under the hood did help obscure the real error.

Finally, if you think this is not useful let's just close and maybe remove vcpkg itself like I mentioned on Discord, we just need consensus on that.

Maybe it's just me. I think we can continue with these changes to the CI, it's an important step towards SDL2. We'll just need a rebase and we can merge

Lgt2x

Ok to merge when README instructions are updated for Linux (maybe mac?) using the new package requirements for VCPKG builds (see my message above)

Lgt2x · 2024-05-06T20:50:11Z

.github/workflows/build.yml

      - name: Install Linux dependencies
        if: ${{ matrix.os.preset == 'linux' }}
        run: |
          sudo apt update
          sudo apt install -y --no-install-recommends \
-            ninja-build cmake g++ libgtest-dev libsdl2-dev libncurses-dev zlib1g-dev libspdlog-dev
+            ninja-build cmake g++ libgtest-dev libsdl2-dev libncurses-dev zlib1g-dev libspdlog-dev \


I can't remember if I asked before but libsdl2-dev, zlib & spdlog should now be provided by VCPKG ?

Yeah, they should. I can change those when I update the PR to add the README

Arcnor force-pushed the enable_vcpkg branch 4 times, most recently from eefa921 to bc654da Compare April 30, 2024 08:09

Arcnor marked this pull request as draft April 30, 2024 08:11

Arcnor force-pushed the enable_vcpkg branch 2 times, most recently from 91ca41d to fbb72d6 Compare April 30, 2024 08:20

Arcnor requested review from winterheart, jcoby and Lgt2x April 30, 2024 08:25

Arcnor marked this pull request as ready for review April 30, 2024 08:29

Lgt2x reviewed Apr 30, 2024

View reviewed changes

winterheart approved these changes Apr 30, 2024

View reviewed changes

JeodC added the workflow Modifications to the github workflows label May 2, 2024

JeodC mentioned this pull request May 3, 2024

Add a copy of the GPL to CI artifacts #281

Closed

13 tasks

JeodC mentioned this pull request May 3, 2024

Enable logger on all CI builds #257

Merged

13 tasks

JeodC added a commit that referenced this pull request May 3, 2024

Add cache to CI

52f4029

This incorporates #242 and #276, and resolves #258

JeodC mentioned this pull request May 3, 2024

Add cache, license, and vcpkg to CI #284

Closed

9 tasks

JeodC closed this May 3, 2024

Arcnor reopened this May 4, 2024

Jayman2000 mentioned this pull request May 4, 2024

Add a copy of the GPL to CI artifacts #287

Merged

13 tasks

Arcnor force-pushed the enable_vcpkg branch 4 times, most recently from 87d7d58 to 0845cb0 Compare May 6, 2024 14:42

enable vcpkg for all platforms on CI

bc95d25

Arcnor force-pushed the enable_vcpkg branch from 0845cb0 to bc95d25 Compare May 6, 2024 14:46

Lgt2x approved these changes May 6, 2024

View reviewed changes

Lgt2x reviewed May 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable vcpkg for all platforms on CI #242

enable vcpkg for all platforms on CI #242

Arcnor commented Apr 30, 2024

Lgt2x left a comment

Lgt2x Apr 30, 2024

Arcnor Apr 30, 2024

Lgt2x May 1, 2024

winterheart left a comment

Arcnor commented Apr 30, 2024

Arcnor commented Apr 30, 2024

Lgt2x commented May 1, 2024 •

edited

Arcnor commented May 3, 2024

Arcnor commented May 4, 2024 •

edited

Jayman2000 commented May 4, 2024

JeodC commented May 4, 2024 •

edited

Jayman2000 commented May 4, 2024

Lgt2x commented May 6, 2024

Lgt2x left a comment

Lgt2x May 6, 2024

Arcnor May 6, 2024

enable vcpkg for all platforms on CI #242

Are you sure you want to change the base?

enable vcpkg for all platforms on CI #242

Conversation

Arcnor commented Apr 30, 2024

Pull Request Type

Description

Related Issues

Screenshots (if applicable)

Checklist

Additional Comments

Lgt2x left a comment

Choose a reason for hiding this comment

Lgt2x Apr 30, 2024

Choose a reason for hiding this comment

Arcnor Apr 30, 2024

Choose a reason for hiding this comment

Lgt2x May 1, 2024

Choose a reason for hiding this comment

winterheart left a comment

Choose a reason for hiding this comment

Arcnor commented Apr 30, 2024

Arcnor commented Apr 30, 2024

Lgt2x commented May 1, 2024 • edited

Arcnor commented May 3, 2024

Arcnor commented May 4, 2024 • edited

Jayman2000 commented May 4, 2024

JeodC commented May 4, 2024 • edited

Jayman2000 commented May 4, 2024

Lgt2x commented May 6, 2024

Lgt2x left a comment

Choose a reason for hiding this comment

Lgt2x May 6, 2024

Choose a reason for hiding this comment

Arcnor May 6, 2024

Choose a reason for hiding this comment

Lgt2x commented May 1, 2024 •

edited

Arcnor commented May 4, 2024 •

edited

JeodC commented May 4, 2024 •

edited