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

games-action/dxx-rebirth: New ebuild revision to support current gcc #16308

Closed
wants to merge 3 commits into from
Closed

games-action/dxx-rebirth: New ebuild revision to support current gcc #16308

wants to merge 3 commits into from

Conversation

dr-diem
Copy link
Contributor

@dr-diem dr-diem commented Jun 18, 2020

The intent of this PR is to update the ebuild for this package such that it will build without error on current gcc (9.3.0). It comprises three packages:

  • an updated dxx-rebirth ebuild based on that contributed to the upstream DXX Rebirth project
  • a dependent package containing some additional free content for Descent 1 (German translation, high-res textures and sampled music from in-period FM synthesisers)
  • a dependent package containing some additional free content for Descent 2 (German translation and sampled music from in-period FM synthesisers)

The existing dxx-rebirth-0.59.100.ebuild package and its unique dependencies (the files) subdir are superseded by this new package.

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @dr-diem
Areas affected: ebuilds
Packages affected: games-action/descent1-freedata, games-action/descent2-freedata, games-action/dxx-rebirth

games-action/descent1-freedata: @gentoo/proxy-maint (new package)
games-action/descent2-freedata: @gentoo/proxy-maint (new package)
games-action/dxx-rebirth: @gentoo/games

Linked bugs

Bugs linked: 724882, 724884, 724886


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added new package The PR is adding a new package. assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Jun 18, 2020
@chewi chewi self-assigned this Jun 28, 2020
Copy link
Member

@chewi chewi left a comment

Choose a reason for hiding this comment

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

I had intended to get this one merged today but I've hit a stumbling block. It's not building with GCC 10. I updated to this just a couple of days ago but it's actually been available for a couple of months now. I can see that there was a GCC 10 fix committed recently. It appears to be a different one so I'm guessing the one I hit has already been fixed?

scons: *** [build/main/similar/main/.d2x-rebirth.net_udp.o] Error 1
similar/main/net_udp.cpp: In function ‘int net_udp_game_connect({anonymous}::direct_join*)’:
similar/main/net_udp.cpp:5844:4: error: ‘%s’ directive output may be truncated writing up to 727 bytes into a region of size 497 [-Werror=format-truncation=]
 5844 |  F("\nConnected to\n\"%s\"\n", netgame->game_name.data()) \
      |    ^

There's also been a new snapshot since this one. Sorry for the delay in looking at this but in my defence, the snapshot was released before this pull request was opened! The snapshot was also made before the recent GCC 10 fix though so perhaps we need another one?

Before running into trouble, I took the liberty of adding newlines to the ends of the files where they were missing. GitHub has highlighted these. I also reworded your commit messages to fit the required character width. Please read up on best practise for commit messages. Finally I folded your most recent commit into the previous one as a fix-up as we don't want the extra noise in the repository.

Strictly speaking, you're supposed to make these commits with repoman commit but I was going to disregard that as I didn't want to hold things up any more. I have checked and it won't mess with the copyright header, despite the warning.

One more thing I just noticed. You've added a bunch of entries to the Manifest for files that haven't been committed like dxx-rebirth-9999.ebuild and also metadata.xml, which is not supposed to be included. repoman commit would have automatically stripped these out. It also would have flagged up the commit message line lengths.

@dr-diem
Copy link
Contributor Author

dr-diem commented Jun 29, 2020

Thanks @chewi

For the gcc 10 issue, 10 is still in testing whilst 9.3.0 is stable - surely we should be compiling with the stable gcc no?

I realised only after the commit that I would be better off naming the ebuild just dxx-rebirth-0.61 without the snapshot suffix so that it could be trivially updated over time; I'll fix the name and target the more recent snapshot. I'll also remove the metadata.xml and not-yet-present-ebuld references from the Manifest.

As for the commit method, ironically I followed Michal Gorny's recent blog post and used his tools rather than repoman for committing; in my case (i.e. as a noob) I will use repoman in future, given the extra checks it performs. Watch this space for a further update!

@chewi
Copy link
Member

chewi commented Jun 29, 2020

For the gcc 10 issue, 10 is still in testing whilst 9.3.0 is stable - surely we should be compiling with the stable gcc no?

"Testing" or "unstable" is relative. There are plenty of people who have never used stable and I'm one of them. We also never mark games stable to reduce the maintenance burden. In any case, we want this to work before it goes stable!

I realised only after the commit that I would be better off naming the ebuild just dxx-rebirth-0.61 without the snapshot suffix so that it could be trivially updated over time; I'll fix the name and target the more recent snapshot. I'll also remove the metadata.xml and not-yet-present-ebuld references from the Manifest.

Please don't do that. If the version never changes then existing installations would never be updated. We also want to be able to tell different upstream versions apart for bug reports and such.

As for the commit method, ironically I followed Michal Gorny's recent blog post and used his tools rather than repoman for committing; in my case (i.e. as a noob) I will use repoman in future, given the extra checks it performs. Watch this space for a further update!

Apologies, you're ahead of me there as I haven't given up the old habits yet. I couldn't tell because unlike repoman, pkgcheck doesn't give any indication that it was used. pkgcheck is certainly better in some ways but perhaps there are some things it doesn't check but repoman does. I'll find out when I give it a try.

@dr-diem
Copy link
Contributor Author

dr-diem commented Jun 29, 2020

I hear you regarding the conceptual nature of what 'stable' really means, but still and all if upstream still isn't 100% compilable against gcc 10 can you consider installing 9.3.0 in parallel and eselect it to test the existing ebuild? It has to be better than the current target (6.5.0!). Also understood regarding retaining the ebuild name; I'll simply make the Manifest tweaks and fix-up after the (repoman!) commit.

As soon as you're happy and have merged this ebuild, my plan is to do similar work for the 9999 ebuild and submit a PR. Only once upstream have formally targeted gcc 10 will I commit a newer snapshot ebuild that we know won't run into compliation issues.

How does all that sound?

@vLKp
Copy link

vLKp commented Jun 30, 2020

The -9999 ebuild works for me right now with gcc-10.1.0, building commit dxx-rebirth/dxx-rebirth@096a678. Based on the error message that @chewi showed, and the surrounding context, you probably need commit dxx-rebirth/dxx-rebirth@df9a2ba, which I pushed May 17.

Only once upstream have formally targeted gcc 10 will I commit a newer snapshot ebuild that we know won't run into compliation issues.

If you mean you want to wait for me to fix all known issues with gcc-10, then I am already done, as far as I know. I can build both with and without editor mode for both d1x and d2x, using gcc-10.1.0, with no errors. If you cannot, please file a Rebirth issue to have me investigate.

If you mean you want me to declare gcc-10 as the minimum supported version, that will be a little while yet. I try to keep support for compiler versions that are readily available on distributions that move a bit more slowly.

@chewi
Copy link
Member

chewi commented Jun 30, 2020

I just mean that GCC 10 should work. We generally don't set minimum GCC supported versions on individual packages.

So it sounds like we're good to go with a new snapshot. I'd be delaying otherwise. It may fix the ancient issues but if the first update to this package in a long time fails to build for many users (many gamers use unstable) then they won't be too happy and I'll be faced with more bug reports. This needs to be right the first time.

@dr-diem
Copy link
Contributor Author

dr-diem commented Jul 1, 2020

I take your point @chewi . I'll create, test and commit a new ebuild based on the commit snapshot you've identified @vLKp , then report back with an updated PR.

@dr-diem
Copy link
Contributor Author

dr-diem commented Jul 3, 2020

Okay, I've brought us up to the 20200615 snapshot which includes the gcc 10 fixes, and added the live ebuild as a bonus. I note that repoman ci strips the reference to the live ebuild from the manifest, but emerge doesn't complain about this omission so I assume it is correct. Over to you once again @chewi!

Add a dependent package for the new dxx-rebirth ebuild, supplying some free content (German translation, high-resolution textures and sampled music from in-period FM synthesisers)

Closes: https://bugs.gentoo.org/724882
Signed-off-by: Ian Silvester <iansilvester@fastmail.fm>
Add a dependent package for the new dxx-rebirth ebuild, supplying some additional free content (German translation and sampled music from in-period FM synthesisers)

Closes: https://bugs.gentoo.org/724884
Signed-off-by: Ian Silvester <iansilvester@fastmail.fm>
Add ebuilds so that the package supports current testing gcc 10, including a live ebuild, and references new free content packages.

Closes: https://bugs.gentoo.org/724886
Signed-off-by: Ian Silvester <iansilvester@fastmail.fm>
@dr-diem
Copy link
Contributor Author

dr-diem commented Jul 3, 2020

This last push was to re-insert references to the 0.59 ebuild into the Manifest that repoman removed (because 0.61 supersedes it as the testing ebuild). Once we are all happy with the new ebuilds I'll remove 0.59 and its various orphaned dependencies.

@gentoo-bot gentoo-bot closed this in 8868a8d Jul 3, 2020
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-07-03 20:07 UTC
Newest commit scanned: 4fc5a05
Status: ❌ broken

New issues caused by PR:
https://qa-reports.gentoo.org/output/gentoo-ci/15b1bf5564/output.html#games-action/descent1-freedata
https://qa-reports.gentoo.org/output/gentoo-ci/15b1bf5564/output.html#games-action/descent2-freedata
https://qa-reports.gentoo.org/output/gentoo-ci/15b1bf5564/output.html#games-action/dxx-rebirth

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/15b1bf5564/output.html

@chewi
Copy link
Member

chewi commented Jul 3, 2020

Phew, I've finally pushed this! Thanks for all your hard work. I've also applied the d1x-rebirth package move.

I added a blank line to 9999 to remove one difference between the ebuilds.

I also removed all *.ebuild lines from the Manifest. repoman removes them because they're not supposed to be in the git version of the tree. They get added to the rsync version of the tree later automatically. If you find that repoman adds them when working in another overlay then add this line to metadata/layout.conf.

thin-manifests = true

@dr-diem
Copy link
Contributor Author

dr-diem commented Jul 3, 2020

Great, thanks for that last tip, and also for your patience as I learnt my way!

NeddySeagoon pushed a commit to NeddySeagoon/gentoo-arm64 that referenced this pull request Jul 4, 2020
Add ebuilds so that the package supports current testing gcc 10, including a live ebuild, and references new free content packages.

Closes: https://bugs.gentoo.org/724886
Signed-off-by: Ian Silvester <iansilvester@fastmail.fm>
Closes: gentoo#16308
Signed-off-by: James Le Cuirot <chewi@gentoo.org>
Copy link
Member

@ulm ulm left a comment

Choose a reason for hiding this comment

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

These ebuilds have clearly been copied from existing ones with a Gentoo copyright header.

Removing credits is really a no-no in the free software community.

@@ -0,0 +1,58 @@
# Copyright 1999-2019 DXX Rebirth project contributors
Copy link
Member

Choose a reason for hiding this comment

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

Yeah sure, "DXX Rebirth project contributors" have worked on this since 1999.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. new package The PR is adding a new package.
Projects
None yet
6 participants