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

app-benchmarks/unigine-superposition: new package, add 1.1 #213

Closed
wants to merge 2 commits into from

Conversation

vitaly-zdanevich
Copy link
Contributor

No description provided.

Copy link
Contributor

@antecrescent antecrescent left a comment

Choose a reason for hiding this comment

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

A bunch of these can be caught using a simple package testing environment.

Path=/opt/unigine-superposition/bin
Name=Superposition Benchmark
GenericName=A GPU Stress test tool from the UNIGINE
Icon=Superposition
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add dodoc Superposition.png somewhere in src_install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, but png is not a documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant to say add doicon Superposition.png in src_install :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already have newicon - is it ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please ignore what I said earlier. Apparently, the Icon Lookup algorithm takes icons of all sizes in the hicolor fallback theme into account.
I was misled by an outdated comment in the desktop eclass.

@vitaly-zdanevich vitaly-zdanevich marked this pull request as draft July 14, 2024 00:07
@vitaly-zdanevich
Copy link
Contributor Author

A bunch of these can be caught using a simple package testing environment.

Interesting that Wiki search is broken https://wiki.gentoo.org/index.php?title=Special%3ASearch&search=frecord-gcc-switches&go=Go

image

@vitaly-zdanevich vitaly-zdanevich marked this pull request as ready for review July 14, 2024 02:32
@ceamac
Copy link
Contributor

ceamac commented Jul 14, 2024

All your commits except the first one lack a Signed-off-by line. Also they are too many :) Please squash them

@vitaly-zdanevich
Copy link
Contributor Author

Squashed.

Copy link
Contributor

@antecrescent antecrescent left a comment

Choose a reason for hiding this comment

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

A few small issues, other than that it looks good from my side!

Please rebase and

  1. create one commit where you add the license to ::guru and add it to the EULA group in profiles/license_groups
  2. create another commit where you add the fixed package

(New licenses should be added in a separate commit before the ebuild that depends on it: https://devmanual.gentoo.org/general-concepts/licenses/index.html#adding-new-licenses)

@vitaly-zdanevich
Copy link
Contributor Author

Done everything.

@antecrescent
Copy link
Contributor

There are merge conflicts with dev. Please fetch the newest commits and rebase. Also the first commit's summary should be: licenses: add Unigine-Superposition-Benchmark-EULA

@vitaly-zdanevich vitaly-zdanevich marked this pull request as draft July 17, 2024 05:57
@vitaly-zdanevich vitaly-zdanevich marked this pull request as ready for review July 17, 2024 06:23
@vitaly-zdanevich
Copy link
Contributor Author

Done.

@antecrescent
Copy link
Contributor

antecrescent commented Jul 17, 2024

The first commit doesn't apply cleanly, because it's not based on dev. You can use the following to properly redo your changes in an up-to-date dev branch.
Also, please refrain from creating merge commits in the future. Gentoo devs will always ask you to rebase anyways.

git fetch --all
git stash # in case there are changed files in your working tree
git reset --hard origin/dev # assuming origin is your upstream remote
git checkout 94ffb55111d025047f09c24ca1a44045e9f8dd8e -- licenses/Unigine-Superposition-Benchmark-EULA # get license from your previous commit into staging area
sed '/^EULA/s/$/ Unigine-Superposition-Benchmark-EULA/' -i profiles/license_groups # redo license_groups addition
git add profiles/license_groups
pkgdev commit -m "licenses: add Unigine-Superposition-Benchmark-EULA"
git cherry-pick a282db6e775266eb0123eefcf2169ea54488e6b6 # re-apply package addition
git push -f
git stash pop

@vitaly-zdanevich vitaly-zdanevich marked this pull request as draft July 18, 2024 11:30
@vitaly-zdanevich vitaly-zdanevich marked this pull request as ready for review July 18, 2024 11:52
@vitaly-zdanevich
Copy link
Contributor Author

Wow, I did it, but still merge commits...

Signed-off-by: Vitaly Zdanevich <zdanevich.vitaly@ya.ru>
Signed-off-by: Vitaly Zdanevich <zdanevich.vitaly@ya.ru>
@antecrescent
Copy link
Contributor

No more complaints from my side. I'd be glad to have another TC acknowledge the changes in this iteration before merging them.

@ceamac
Copy link
Contributor

ceamac commented Jul 19, 2024

LGTM

gentoo-bot pushed a commit that referenced this pull request Jul 19, 2024
Signed-off-by: Vitaly Zdanevich <zdanevich.vitaly@ya.ru>
Closes: #213
Reviewed-by: Viorel Munteanu <ceamac@gentoo.org>
Signed-off-by: Lucio Sauer <watermanpaint@posteo.net>
@ceamac ceamac closed this Jul 19, 2024
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.

4 participants