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

Adding top-level CPACK option #700

Merged
merged 6 commits into from
May 2, 2020
Merged

Conversation

kai4785
Copy link
Contributor

@kai4785 kai4785 commented Apr 13, 2020

And some sane defaults for deb/rpm packaging

With out trying to compete with the PPA packaging over PR #547 , this makes a sane debian package from cpack with out too much hassle. The '7Z' generator seemed .... incomplete, stuffing everything at the top level. Looks like it's only used for PPC? It shouldn't be hard to preserve the old behavior, but it seemed .... incomplete.

And some sane defaults for deb/rpm packaging
RENAME "${project_name}.png"
)
install(FILES "${PROJECT_SOURCE_DIR}/Packaging/resources/CharisSILB.ttf"
DESTINATION "share/fonts/truetype"
Copy link
Member

Choose a reason for hiding this comment

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

the game doesn't look for the font here, are you sure this will work?

Copy link
Contributor

Choose a reason for hiding this comment

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

On our own branch for Amiga, we have added set(TTF_FONT_PATH \"LiberationSerif-Bold.ttf\")

Shouldn't it be possible to check if target == package change font-path to /usr/share/fonts/truetype etc?

Copy link
Contributor Author

@kai4785 kai4785 Apr 13, 2020

Choose a reason for hiding this comment

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

I was emulating what the fedora rpm was doing. Where does the source look for it currently? Seems like a small-ish change for packaging to match the code, but seems like a more expected behavioral change for the code to look for it in a "standard" location.
Also, when does it get used? Because the game seems to work with or with out the font.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh, the fedora spec file has this line:
export CXXFLAGS="-DTTF_FONT_PATH=\"/usr/share/fonts/truetype/CharisSILB.ttf\""
I missed that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a small patch that splits TTF_FONT_PATH into TTF_FONT_NAME and TTF_FONT_DIR so I could search for TTF_FONT_NAME in 3 places:

  1. Current directory
  2. TTF_FONT_DIR
  3. #ifdef __linux__ "/usr/share/fonts/truetype"
    It should preserve the current behavior while allowing the default build on Linux to search for the TTF_FONT_NAME in a known good location.

Copy link
Member

Choose a reason for hiding this comment

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

The font is used in the credits and any error message.

Your patch looks like a decent solution.

@AJenbo
Copy link
Member

AJenbo commented Apr 13, 2020

Nice this looks to mostly solve #212

Do you think there is any reason to keep https://github.com/diasurgical/devilutionX/tree/master/Packaging/debian and https://github.com/diasurgical/devilutionX/tree/master/Packaging/fedora around with this in place?

The PPC build is failing with this change, the 7Z was just setup to replicate the generic package that i normally do manually at each release. I really like the idea of having proper distribution packages with installers and shortcuts implemented, but the downside is that it inflates the number of builds i have to do each release, and makes the list of downloads rather long when considering Linux only makes up 25% of downloads. Any suggestions for how to handle this?

@MBeijer
Copy link
Contributor

MBeijer commented Apr 13, 2020

CPack can handle multiple targets per build, as long as it's for the same platform/arch.

So it can make rpm, deb and tgz packages for x86_64 at the same time. Just add cmake --build . --target package for ppc, x86, etc for linux and that should take care of that.

I have also looked into githubs API, and it looks like you can publish packages to a release automatically in the build process and attach the packages to the release. :)

@MBeijer
Copy link
Contributor

MBeijer commented Apr 13, 2020

And yes, this would most likely solve #212

Amiga will have to settle for ZIP format for packaging at this point. Adding new CPack generators aren't as easy as I originally thought.

Perhaps adding NSIS / WIX generator for Windows target would be a nice touch.

@AJenbo
Copy link
Member

AJenbo commented Apr 13, 2020

CPack can handle multiple targets per build, as long as it's for the same platform/arch.

Well my issue is more with crowding the download section. But I guess we could just zip all variants in one big devilutionx-linux.7z and people can pick cpu and dist from the folders once downloaded.

I have also looked into githubs API, and it looks like you can publish packages to a release automatically in the build process and attach the packages to the release. :)

Yeah we where using this for MacOS but it stopped working for some reason :( It's a bit of a pain as I only have limtied access to a Mac.

@AJenbo
Copy link
Member

AJenbo commented Apr 13, 2020

Perhaps adding NSIS / WIX generator for Windows target would be a nice touch.

I would definitely prefer WIX over NSIS, I used to like NSIS better back in my Windows days, but the fact that it outputs .exe means that every piece of antivirus screams at it at this point :(

One huge issue here though is that WIX is only available when building on Windows, and I don't.

@kai4785
Copy link
Contributor Author

kai4785 commented Apr 13, 2020

Do you think there is any reason to keep [Packaging/debian and Packaging/fedora] in place?

Not if this "works" : )

CPack can handle multiple targets per build, as long as it's for the same platform/arch.

If we are OK packaging the same binary from the same build for different platforms, my Ubuntu 18.04 system is capable of producing .deb and .rpm files. I don't have a desktop fedora running around, but I am intensely familiar with building for RedHat systems, and the rpm requires and provides "look" proper. It would be a matter of building up the "list" of generators, and calling cpack.

The PPC build is failing with this change

Whoopsie, I don't include CPack or set the 7Z generator unless you specify -DCPACK=On during the build, which I didn't do. Fixable for sure. If you like it enough to be on for every build, then we can just set CPACK to On, and create the generator list based on the environment we're in. I'm just not familiar enough with the project to know what direction to go.

@AJenbo
Copy link
Member

AJenbo commented Apr 13, 2020

Unfortunately, we don't know the breakdown of distros or what install method they prefer (Aur/PPA, compile from source). For Linux, I think we should go with DEB, RPM and 7Z since both Debian and Fedora users cared enough about it to contribute a setup for it, so I guess we can conclude that users of other distros are happy enough with the alternatives.

So this would be the full list of potential builds I would do for a release:
Linux x86_64 .deb
Linux x86_64 .rpm
Linux x86_64 .7z
Linux armhf .deb
Linux x86_64 .rpm
Linux x86_64 .7z
Linux armhf .deb
Linux armhf .rpm
Linux armhf .7z
Linux ppc64le .deb
Linux ppc64le .rpm
Linux ppc64le .7z
Windows x86 .zip
macOS x86_64 .dmg
retrogamer .opk
amiga .zip
switch .zip

(Popularity will influence how many I actually prebuild)

FreeBSD, OpenBSD, Haiku and Clockwork PI pretty much haver there own distribution channel.

@kai4785
Copy link
Contributor Author

kai4785 commented Apr 16, 2020

I think that's a tenable position, and it matches my experience as well. For an application with such a simple set of dependencies, the .deb and .rpm packages generated on a debian system should "just work" for most people.

So how to drive this to ground?
We have the outstanding .ttf path issue, I can look into that.
Gotta fix the ppc build by generating the .7z archive. I will give a whack at making sure we generate the proper packages during the appropriate builds. I may need some guidance there.
And I think generally we set CPACK to ON, don't you think? I can make it a RELEASE_OPTION if you like that. Or do you even want it to be optional?

This initializes CMAKE_CXX_FLAGS_RELEASE during project() correctly. With out
this, changes to CMakeLists.txt may result in a complete rebuild because
CMAKE_BUILD_TYPE wasn't in the cache before, and wasn't respected by project(),
but now it is.

https://cmake.org/pipermail/cmake/2008-September/023808.html
@AJenbo
Copy link
Member

AJenbo commented Apr 16, 2020

Yeah I would prefer it to not be on for debug builds.

For current .deb devilutionx is being build with a differne look up bath for the ttf. Not sure if that can work with out rebuilding for each package type (that's technically how it is being done now).

@AJenbo AJenbo merged commit 791aa60 into diasurgical:master May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants