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

x11-apps/intel-gpu-tools: version bump to 1.22 bug #655224 #8729

Closed
wants to merge 1 commit into from

Conversation

cgbowman
Copy link
Contributor

@cgbowman cgbowman commented Jun 4, 2018

This update adopts the meson build system being used
by intel-gpu-tools.

The included patch adds KBL and ICL GPU PCI IDs.

Signed-off-by: Casey Bowman casey.g.bowman@intel.com

@gentoo-bot
Copy link

Pull Request assignment

Areas affected: ebuilds
Packages affected: x11-apps/intel-gpu-tools

x11-apps/intel-gpu-tools: @gentoo/x11

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and ping us to reset the assignment.

If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers.

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

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Jun 4, 2018
@mattst88
Copy link
Contributor

mattst88 commented Jun 5, 2018

Thanks!

So, quite a few issues jump out at me. Let me just call out what I see.

The Manifest file was regenerated using the old hash functions. I'm guessing that your version of sys-apps/portage is old -- older than 2.3.13-r1 I think? Please update portage and rerun repoman manifest. The only change to Manifest should be the addition of a line for v1.22

Please move sys-libs/libunwind in RDEPEND to the line before sys-process/procps (unconditional dependencies typically go before conditional ones), and I see that libunwind was made unconditional between v1.20 and v1.21 upstream. Also remove the now unused "unwind" from IUSE.

In X86_DEPEND, it looks like you accidentally reverted an upstream change to depend on x11-base/xorg-proto. Please restore that.

In upstream commit 865a47ca2b93b208ba016 we gained a new dependency on dev-util/peg. The i-g-t build system needs 'leg -P' which isn't available in the newest version packaged. I'll work on doing a version bump of that, and once that's done we'll need to add >=dev-util/leg-0.1.18 to X86_DEPEND I think.

src_install() is now unneeded. Calling meson_src_install is the default for ebuilds inheriting meson.eclass, so that whole function can be removed from the ebuild.

I see that there are some xmlrpc dependencies in meson.build for chamelium support. Do you care to support that?

I see that the test-programs USE flag was dropped. I do not see a way to disable tests like --disable-tests in configure.ac. I've asked in #intel-gfx and will let you know if I learn anything.

I'm rather startled to see all configuration options simply removed. I see meson_options.txt contains only one thing (and not one we offered). I'm honestly not sure what to do about this... without the ability to disable dependencies (even if they exist) all of the optional deps will be "automagic" (https://wiki.gentoo.org/wiki/Project:Quality_Assurance/Automagic_dependencies).

@mattst88 mattst88 self-assigned this Jun 5, 2018
@cgbowman
Copy link
Contributor Author

cgbowman commented Jun 5, 2018

Thanks Matt for the extensive review; as a somewhat new user to Gentoo, I really appreciate it. :)

I'll address each of your points in the next update.

I've since updated my version of sys-apps/portage to 2.3.40.

For Chamelium support, it's mainly used in ChromeOS for simulating tasks like display hotplugging, so my guess is that it would be used very little outside of ChromeOS. Still, since it's included, it's best to give the option for support, so I'll see if I can add a conditional dependency with a USE flag: enable_chamelium.

As for the configuration options, I was surprised as well; I was looking to how you ported libdrm from autotools to meson as a reference and saw the vast differences in meson_options.txt file.

Most likely, we'll need to add an epatch in the future for build configuration options.

@cgbowman cgbowman force-pushed the intel_gpu_tools_1_22_update branch from 6390f12 to 8be4254 Compare June 6, 2018 01:28
@cgbowman
Copy link
Contributor Author

cgbowman commented Jun 6, 2018

I see that I forgot to add the USE flag description for "enable_chamelium" to the metadata.xml file, I'll make sure to upload that change tomorrow morning.

@mattst88
Copy link
Contributor

mattst88 commented Jun 6, 2018

Thanks a bunch!

I'd just make the USE flag named "chamelium" without the "enable_" prefix.

@cgbowman cgbowman force-pushed the intel_gpu_tools_1_22_update branch from 8be4254 to 5893ef8 Compare June 6, 2018 17:32
@cgbowman
Copy link
Contributor Author

cgbowman commented Jun 6, 2018

Changes made. I took your suggestion to remove the "enable_" prefix on the new USE flag.

Do we want this pull request to wait on the peg 0.1.18 rev bump?

@mattst88
Copy link
Contributor

mattst88 commented Jun 8, 2018

As we discussed in person, I think the best path forward (unfortunately) is to go back to autotools for 1.22. I'd suggest using your meson-enabled ebuild to make a git ebuild. Take a look at media-libs/libepoxy/libepoxy-9999.ebuild for an example (you can cut out all the multilib stuff).

Sorry again -- I had no idea that the Meson build system in i-g-t wasn't configurable :(

Also dev-util/peg-0.1.18 is in tree, so it's ready to be used :)

@cgbowman
Copy link
Contributor Author

cgbowman commented Jun 8, 2018

With the reverts back to autotools, a 9999 version, and the testing for both, I'll be looking to push another version sometime early next week.

@cgbowman
Copy link
Contributor Author

Since we are concerned about build options, I started adding more options to the 1.22 ebuild because options are nice. :)

However, when I saw an option for enabling shader debugging and tested it out, it seemed that the feature fails compilation because a couple calls to "intel_register_access_init" aren't using the DRM device FD parameter in the debugger code, (Might need to file a bug).

An example:

/var/tmp/portage/x11-apps/intel-gpu-tools-1.22/work/intel-gpu-tools-1.22/debugger/debug_rdata.c:138:2: error: too few arguments to function 'intel_register_access_init'
intel_register_access_init(pci_dev, 1);
^
In file included from /var/tmp/portage/x11-apps/intel-gpu-tools-1.22/work/intel-gpu-tools-1.22/debugger/debug_rdata.c:31:0:
/var/tmp/portage/x11-apps/intel-gpu-tools-1.22/work/intel-gpu-tools-1.22/lib/intel_io.h:39:5: note: declared here
int intel_register_access_init(struct pci_device *pci_dev, int safe, int fd);
^

Looks like it's been over a year since this functionality was in place, so perhaps this option isn't being used much anyway?

commit 301ad44cdf1b868b1ab89096721da91fa8541fdc
Author: Tomeu Vizoso tomeu.vizoso@collabora.com
AuthorDate: Thu Mar 2 10:37:11 2017 +0100
Commit: Tomeu Vizoso tomeu.vizoso@collabora.com
CommitDate: Tue Mar 21 15:50:54 2017 +0100

lib: Open debugfs files for the given DRM device

Would we want to just drop options like this?

Otherwise, I'll just leave a gtk-doc flag available so that users can generate some documentation if they'd like.

Other than these changes, the 1.22 ebuild and the 9999 ebuild are about ready, so I'll upload the updated patch tomorrow.

This updates intel-gpu-tools to version 1.22,
which will use autotools because the meson build
options are essentially nonexistent, compared
to the autotools configuration scripts.

This change also adds a live version that uses
meson instead of autotools.

A patch was included to add KBL and ICL GPU PCI IDs.

Closes: https://bugs.gentoo.org/655224

Signed-off-by: Casey Bowman <casey.g.bowman@intel.com>
@cgbowman cgbowman force-pushed the intel_gpu_tools_1_22_update branch from 5893ef8 to 7ff388c Compare June 13, 2018 16:55
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2018-06-13 17:13 UTC
Newest commit scanned: 7ff388c
Status: ✅ good

No issues found

@mattst88
Copy link
Contributor

Thanks a bunch! I've merged this now. I made a couple of small changes -- notably I split it into two separate commits (one adding v1.22 and a second adding the git ebuild since I remembered there was a request bug for that -- bug 390943) and changed the gtk-doc USE flag to just doc to match other packages.

Thanks so much! Really happy to have contributions from you and your team!

gentoo-bot pushed a commit that referenced this pull request Jun 13, 2018
Closes: https://bugs.gentoo.org/390943
Closes: #8729
Signed-off-by: Casey Bowman <casey.g.bowman@intel.com>
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). no bug found No Bug/Closes found in the commits.
Projects
None yet
4 participants