-
Notifications
You must be signed in to change notification settings - Fork 80
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
Update resource-building scripts for future compatibility. #656
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also update the appveyor.yml to install/use CairoSVG instead of PyGTK?
Yes! I knew I forgot something. I'll add that. |
3bc65b3
to
1324efc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing requirements.txt with the contents:
cairosvg==1.0.22
pillow
We aren't currently using that method. Are you requesting that it be added to this PR? |
Yes, we want want to expose on master the packages needed for these scripts to work on Py2.7. |
Alright, will do. |
857b78e
to
3d1dcfd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be nit-picky, but I feel like the requirements.txt should live in the same directory as the resource generation script. The game scripts, for example, don't require the modules listed therein.
@@ -12,6 +12,7 @@ jobs: | |||
sudo apt install cmake libcurl4-openssl-dev libopenal-dev libssl-dev \ | |||
libogg-dev libopus-dev libspeex-dev uuid-dev libvpx-dev \ | |||
libvorbis-dev qtbase5-dev | |||
sudo pip install -r ${GITHUB_WORKSPACE}/requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too bad the resource.dat is never built on linux-ci since it's part of the plClient build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not currently, but hopefully that will change. I know the scripts run on there because I've done so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the requirements file allows specifications based on environment (such as OS), it makes sense to add this to both build systems for consistency (and to watch for accidental breakages).
3d1dcfd
to
6928beb
Compare
I considered this, but then the question is when is this step executed? I think this is redundant. We currently install all dependencies in the apt-get/vcpkg steps, and let CMake work out what's enabled and available. This is consistent with that approach. If these Python modules are unavailable, or not requested in the build, CMake will respond appropriately. Also, a dependency-installation step that requires a file nested deep inside the project tree seems prone to breakage. Unless we want to have CMake handle running PIP as a custom command inside the step that sets I see this file as a project-wide dependencies install list, which just happens to only be used by a single sub-project currently. That would appear to be the general intent of it. We can also nest them... perhaps you'd prefer that? If this |
6928beb
to
6d8b415
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This updates the resource.dat creation to render the SVG assets using a newer, maintained Python module "CairoSVG" instead of relying on the Pycairo/rsvg pair which is no longer actively maintained for Python on Windows. Also included are a few Python 3.x+ fixes which should allow the scripts and build system to function with the newer interpreter.
This is a foundation for fixes coming in #650.