Descent3

Descent3 copied to clipboard

Pull Request Type

• [x] GitHub Workflow changes
• [ ] Documentation or Wiki changes
• [x] Build and Dependency changes
• [ ] Runtime changes
  • [ ] Render changes
  • [ ] Audio changes
  • [ ] Input changes
  • [ ] Network changes
  • [ ] Other changes

Description

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

Related Issues

Relates to #241

Screenshots (if applicable)

Checklist

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

Additional Comments

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.

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.

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")

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.

@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.

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'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.

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.

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

this PR needs a rework with the removal of spdlog.

Closing as inactive and outdated, many 3rd party changes since this was first open. Still, it's an idea I'd like to see implemented