Bug 1415143 - Review Request: egl-wayland - Wayland EGL External Platform library
Summary: Review Request: egl-wayland - Wayland EGL External Platform library
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Simone Caronni
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-01-20 12:03 UTC by leigh scott
Modified: 2017-03-27 07:36 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-03-27 07:36:39 UTC
Type: ---
negativo17: fedora-review+


Attachments (Terms of Use)

Description leigh scott 2017-01-20 12:03:48 UTC
Spec URL: https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/1/egl-wayland.spec

SRPM URL: https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/1/egl-wayland-1.0.0-0.1.git743d702.fc26.src.rpm

Description: Wayland EGL External Platform library

Fedora Account System Username: leigh123linux

Comment 1 Simone Caronni 2017-01-20 12:20:32 UTC
eglexternalplatform is generic, but tsn't this the specific library for the Nvidia binary drivers? Shouldn't it go in RPMFusion?

Comment 2 leigh scott 2017-01-20 12:27:04 UTC
(In reply to Simone Caronni from comment #1)
> eglexternalplatform is generic, but tsn't this the specific library for the
> Nvidia binary drivers? Shouldn't it go in RPMFusion?

It has a MIT licence so I doubt it qualifies for rpmfusion.

Comment 4 Hans de Goede 2017-02-01 11:59:53 UTC
About the email discussion about the etc/egl/egl_external_platform.d and /usr/share/egl/egl_external_platform.d directories and dropping a json file there so that eglexternalplatform can actually find egl-wayland (if I've understood things correctly):

I'm going to need some info on how all these new bits fit together.

So egl-wayland is a (the wayland) platform provider for eglexternalplatform right? Then it should also runtime depend on eglexternalplatform, either through auto .so.x dependencies, or explicit in the spec I assume ?

Then to me it would make sense for the main eglexternalplatform pkg to create/own the /etc/egl/egl_external_platform.d and /usr/share/egl/egl_external_platform.d directories.

I agree that if eglexternalplatform finds its platform providers / plugins through json files, that the json file should be part of the package which contains the .so file which contains the platform-provider implementation, so in this specific case the main egl-wayland package.

About the egl-wayland package, I don't think it make sense to have a -devel there as there are no headers and this is a plugin, depending on how the json to load it refers to the .so, either drop the .so symlink or if it is necessary make it part of the main pkg and add a comment to the %files section why it is there.

Comment 5 Simone Caronni 2017-02-01 12:19:39 UTC
(In reply to Hans de Goede from comment #4)
> About the email discussion about the etc/egl/egl_external_platform.d and
> /usr/share/egl/egl_external_platform.d directories and dropping a json file
> there so that eglexternalplatform can actually find egl-wayland (if I've
> understood things correctly):

Not exactly. So, the Nvidia driver loads this library through their GL library:

$ strings libEGL_nvidia.so.378.09  | grep egl_external
/etc/egl/egl_external_platform.d:/usr/share/egl/egl_external_platform.d

> I'm going to need some info on how all these new bits fit together.
> 
> So egl-wayland is a (the wayland) platform provider for eglexternalplatform
> right? Then it should also runtime depend on eglexternalplatform, either
> through auto .so.x dependencies, or explicit in the spec I assume ?

That folder is in the specification of eglexternalplatform. The Nvidia EGL library adheres to this, and tries to load the library from that folder with a json file:

$ cat 10_nvidia_wayland.json
{
    "file_format_version" : "1.0.0",
    "ICD" : {
        "library_path" : "libnvidia-egl-wayland.so.1"
    }
}

> Then to me it would make sense for the main eglexternalplatform pkg to
> create/own the /etc/egl/egl_external_platform.d and
> /usr/share/egl/egl_external_platform.d directories.

This makes sense, but since eglexternalplatform is just a devel package with no library or anything else itself, I'm not sure if it makes sense to create an additional subpackage out of eglexternalplatform just to own an empty directory.

Can we just maybe put it in the libglvnd-egl subpackage maybe with a note in the SPEC file? That package will always be installed on a system where the Nvidia proprietary glvnd/EGL library is installed.

> I agree that if eglexternalplatform finds its platform providers / plugins
> through json files, that the json file should be part of the package which
> contains the .so file which contains the platform-provider implementation,
> so in this specific case the main egl-wayland package.

Exactly, so the full file list of egl-wayland should be as follows:

$ rpm -qpl egl-wayland-1.0.0-0.2.20170120git743d702.fc26.x86_64.rpm
/usr/lib64/libnvidia-egl-wayland.so.1
/usr/lib64/libnvidia-egl-wayland.so.1.0.0
/usr/share/doc/egl-wayland
/usr/share/doc/egl-wayland/README.md
/usr/share/egl/egl_external_platform.d/10_nvidia_wayland.json

> About the egl-wayland package, I don't think it make sense to have a -devel
> there as there are no headers and this is a plugin, depending on how the
> json to load it refers to the .so, either drop the .so symlink or if it is
> necessary make it part of the main pkg and add a comment to the %files
> section why it is there.

Makes sense.

Comment 6 Simone Caronni 2017-02-01 12:24:39 UTC
Summing up, my proposal would be:

- empty egl platform folders in some common EGL package used by all EGL implementations like Mesa/Nvidia -> libglvn-egl, for example
- no json file in the Nvidia driver library package, as it should not try to load it for example in RHEL 7 builds
- json file included in the egl-wayland library package. If it's installed because pulled in by a dependency in the Nvidia driver library package (so no RHEL), it gets loaded

Comment 7 leigh scott 2017-02-01 12:52:18 UTC
(In reply to Simone Caronni from comment #6)
> Summing up, my proposal would be:
> 
> - empty egl platform folders in some common EGL package used by all EGL
> implementations like Mesa/Nvidia -> libglvn-egl, for example

Is any other EGL implementation going to use the /usr/share/egl/egl_external_platform.d directory?
If not maybe egl-wayland should own it and add a -libs subpackage for libnvidia-egl-wayland.so.*


> - no json file in the Nvidia driver library package, as it should not try to
> load it for example in RHEL 7 builds
> - json file included in the egl-wayland library package. If it's installed
> because pulled in by a dependency in the Nvidia driver library package (so
> no RHEL), it gets loaded

What license does the 10_nvidia_wayland.json file have?, maybe it would be better to keep it in nvidia driver package.

Comment 8 Hans de Goede 2017-02-01 13:13:33 UTC
Hi,

Ok, so we've 2 separate issues:

1) Where to put the json file, since the json files points to libnvidia-egl-wayland.so.1 to me the only sensible place is to package it together with libnvidia-egl-wayland.so.1 . As for the license, IANAL but the contents is purely functional and as such not copyrightable. Also nvidia is usually happy to license all the various non binary only bits under MIT, so IMHO it is safe to assume that this file is MIT licensed too.

2) We need some package to create / own the egl/egl_external_platform.d/ directories and this should not be nvidia specific since in theory in the future there could be other providers. I'm not entirely happy about this but I've to agree that the libglvnd-egl sub-package seems like the best place for this. If everyone agrees I can make this change, and we can move forward regardless as unowned directories are not really a big problem.

Regards,

Hans

Comment 9 leigh scott 2017-02-01 13:47:29 UTC
- Add loader directory to common sub-package
- Move libs to sub-package
- Package needs to to multi-lib compliant


Spec URL: https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/3/egl-wayland.spec

SRPM URL: https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/3/egl-wayland-1.0.0-0.3.20170120git743d702.fc26.src.rpm

Comment 10 leigh scott 2017-02-01 13:56:56 UTC
(In reply to Hans de Goede from comment #8)
> Hi,
> 
> Ok, so we've 2 separate issues:
> 
> 1) Where to put the json file, since the json files points to
> libnvidia-egl-wayland.so.1 to me the only sensible place is to package it
> together with libnvidia-egl-wayland.so.1 . 

I believe the driver common (xorg-x11-drv-nvidia or nvidia-driver) should provide the json file as this file isn't really suitable for the common subpackage and can't go in the libs package as it would break multi arch install




> 2) We need some package to create / own the egl/egl_external_platform.d/
> directories and this should not be nvidia specific since in theory in the
> future there could be other providers. I'm not entirely happy about this but
> I've to agree that the libglvnd-egl sub-package seems like the best place
> for this. If everyone agrees I can make this change, and we can move forward
> regardless as unowned directories are not really a big problem.
>

The common sub-package addresses this issue

> Regards,
> 
> Hans

Comment 11 Hans de Goede 2017-02-01 13:59:35 UTC
(In reply to leigh scott from comment #10)
> (In reply to Hans de Goede from comment #8)
> > Hi,
> > 
> > Ok, so we've 2 separate issues:
> > 
> > 1) Where to put the json file, since the json files points to
> > libnvidia-egl-wayland.so.1 to me the only sensible place is to package it
> > together with libnvidia-egl-wayland.so.1 . 
> 
> I believe the driver common (xorg-x11-drv-nvidia or nvidia-driver) should
> provide the json file as this file isn't really suitable for the common
> subpackage and can't go in the libs package as it would break multi arch
> install

The json file only specifies the relative filename, not the entire path, so it wil be indentical on systems which have both lib and lib64 and as such there is no multi-lib issues. Please just add the json file to the -libs sub-package.

> > 2) We need some package to create / own the egl/egl_external_platform.d/
> > directories and this should not be nvidia specific since in theory in the
> > future there could be other providers. I'm not entirely happy about this but
> > I've to agree that the libglvnd-egl sub-package seems like the best place
> > for this. If everyone agrees I can make this change, and we can move forward
> > regardless as unowned directories are not really a big problem.
> >
> 
> The common sub-package addresses this issue

Looks good / works for me. Please do also add the %{sysconfdir} version of the dir (/etc/...)

Comment 12 leigh scott 2017-02-01 14:10:31 UTC
(In reply to Hans de Goede from comment #11)
> (In reply to leigh scott from comment #10)
> > (In reply to Hans de Goede from comment #8)
> > > Hi,
> > > 
> > > Ok, so we've 2 separate issues:
> > > 
> > > 1) Where to put the json file, since the json files points to
> > > libnvidia-egl-wayland.so.1 to me the only sensible place is to package it
> > > together with libnvidia-egl-wayland.so.1 . 
> > 
> > I believe the driver common (xorg-x11-drv-nvidia or nvidia-driver) should
> > provide the json file as this file isn't really suitable for the common
> > subpackage and can't go in the libs package as it would break multi arch
> > install
> 
> The json file only specifies the relative filename, not the entire path, so
> it wil be indentical on systems which have both lib and lib64 and as such
> there is no multi-lib issues. Please just add the json file to the -libs
> sub-package.

Try counting to ten then think about the file conflict it would generate :-)

egl-wayland-libs.x86_64
egl-wayland-libs.i686

both try and install /usr/share/egl/egl_external_platform.d/10_nvidia_wayland.json

Comment 13 leigh scott 2017-02-01 14:16:16 UTC
(In reply to leigh scott from comment #9)
> - Add loader directory to common sub-package
> - Move libs to sub-package
> - Package needs to to multi-lib compliant
> 
> 
> Spec URL:
> https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/3/egl-wayland.
> spec
> 
> SRPM URL:
> https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/3/egl-wayland-
> 1.0.0-0.3.20170120git743d702.fc26.src.rpm

%{_sysconfdir}/egl/egl_external_platform.d/ patch added

Comment 14 leigh scott 2017-02-01 14:49:09 UTC
Another reason for including the 10_nvidia_wayland.json in the nvidia driver package is the lib would be advertising egl_external_platform.d capability when it has no chance of working due to missing nvidia driver.

Comment 15 Hans de Goede 2017-02-01 16:00:46 UTC
HI,

(In reply to leigh scott from comment #12)
> (In reply to Hans de Goede from comment #11)
> > (In reply to leigh scott from comment #10)
> > > (In reply to Hans de Goede from comment #8)
> > > > Hi,
> > > > 
> > > > Ok, so we've 2 separate issues:
> > > > 
> > > > 1) Where to put the json file, since the json files points to
> > > > libnvidia-egl-wayland.so.1 to me the only sensible place is to package it
> > > > together with libnvidia-egl-wayland.so.1 . 
> > > 
> > > I believe the driver common (xorg-x11-drv-nvidia or nvidia-driver) should
> > > provide the json file as this file isn't really suitable for the common
> > > subpackage and can't go in the libs package as it would break multi arch
> > > install
> > 
> > The json file only specifies the relative filename, not the entire path, so
> > it wil be indentical on systems which have both lib and lib64 and as such
> > there is no multi-lib issues. Please just add the json file to the -libs
> > sub-package.
> 
> Try counting to ten then think about the file conflict it would generate :-)
> 
> egl-wayland-libs.x86_64
> egl-wayland-libs.i686

That is not how multi-lib works, if the file contents are the same there is no conflict, otherwise all the /usr/share/doc/libfoo/COPYING files would all conflict between there 32 and 64 bit version.

(In reply to leigh scott from comment #14)
> Another reason for including the 10_nvidia_wayland.json in the nvidia driver
> package is the lib would be advertising egl_external_platform.d capability
> when it has no chance of working due to missing nvidia driver.

The whole idea of the external_platform spec is that there might be other users of the platform-providers, sure currently there are no other users, but that also means that no other code will look at the json files so it won't hurt.

Comment 16 leigh scott 2017-02-01 17:20:25 UTC
(In reply to Hans de Goede from comment #15)
> HI,
> 
> (In reply to leigh scott from comment #12)
> > (In reply to Hans de Goede from comment #11)
> > > (In reply to leigh scott from comment #10)
> > > > (In reply to Hans de Goede from comment #8)
> > > > > Hi,
> > > > > 
> > > > > Ok, so we've 2 separate issues:
> > > > > 
> > > > > 1) Where to put the json file, since the json files points to
> > > > > libnvidia-egl-wayland.so.1 to me the only sensible place is to package it
> > > > > together with libnvidia-egl-wayland.so.1 . 
> > > > 
> > > > I believe the driver common (xorg-x11-drv-nvidia or nvidia-driver) should
> > > > provide the json file as this file isn't really suitable for the common
> > > > subpackage and can't go in the libs package as it would break multi arch
> > > > install
> > > 
> > > The json file only specifies the relative filename, not the entire path, so
> > > it wil be indentical on systems which have both lib and lib64 and as such
> > > there is no multi-lib issues. Please just add the json file to the -libs
> > > sub-package.
> > 
> > Try counting to ten then think about the file conflict it would generate :-)
> > 
> > egl-wayland-libs.x86_64
> > egl-wayland-libs.i686
> 
> That is not how multi-lib works, if the file contents are the same there is
> no conflict, otherwise all the /usr/share/doc/libfoo/COPYING files would all
> conflict between there 32 and 64 bit version.
> 

Ok, you learn something new everyday.


- Add 10_nvidia_wayland.json to libs sub-package


Spec URL: https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/4/egl-wayland.spec


SRPM URL: https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/4/egl-wayland-1.0.0-0.4.20170120git743d702.fc26.src.rpm

Comment 17 Hans de Goede 2017-02-01 18:35:11 UTC
(In reply to leigh scott from comment #16)
> - Add 10_nvidia_wayland.json to libs sub-package
> 
> 
> Spec URL:
> https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/4/egl-wayland.
> spec
> 
> 
> SRPM URL:
> https://leigh123linux.fedorapeople.org/pub/review/egl-wayland/4/egl-wayland-
> 1.0.0-0.4.20170120git743d702.fc26.src.rpm

Thanks, looks good now, only you still have an unnecessary -devel sub-package, nothing is going to compile time link aginst the .so so you can simply remove the .so symlink in %install and drop the -devel.

With that fixed this looks good to me and can be approved (after a full review is done, but I do not believe that will catch anything else).

Comment 19 Hans de Goede 2017-02-01 19:32:39 UTC
Looks good now, I spotted one typo (but then can be fixed on import):

%description    common
Commom files for %{name}.

s/Commom/Common/

I'm not going to do a full review as this is assigned to Simone, but from my pov this looks good now.

Comment 20 Nicolas Chauvet (kwizart) 2017-02-01 20:28:06 UTC
I don't get the point of splitting -libs and -common (without even a main package).
-libs isn't related to multi-libs compliance, specially as there is no main sub-package. It's totally arbitrary sub-package, multilibs compliance is a totally different thing.
- I don't see much point for a -common sub-package with only directories (can be merged together).

I haven't read the whole thread, (and havent's tested anything yet), so I won't comment on where best the json file should be located.

Comment 21 Simone Caronni 2017-02-02 07:45:15 UTC
(In reply to Nicolas Chauvet (kwizart) from comment #20)
> I don't get the point of splitting -libs and -common (without even a main
> package).
> -libs isn't related to multi-libs compliance, specially as there is no main
> sub-package. It's totally arbitrary sub-package, multilibs compliance is a
> totally different thing.

Agree, we should just have the egl-wayland package with the library.

> - I don't see much point for a -common sub-package with only directories
> (can be merged together).
>
> I haven't read the whole thread, (and havent's tested anything yet), so I
> won't comment on where best the json file should be located.

Yes and no. It's correct that there should not be a separate package for it, but actually Hans and my proposal was to put the directories in libglvnd-egl and the json file in the package. Please read comment 6 and 8.

@Hans, am I correct?

Comment 22 leigh scott 2017-02-02 08:17:56 UTC
(In reply to Simone Caronni from comment #5)
> Exactly, so the full file list of egl-wayland should be as follows:
> 
> $ rpm -qpl egl-wayland-1.0.0-0.2.20170120git743d702.fc26.x86_64.rpm
> /usr/lib64/libnvidia-egl-wayland.so.1
> /usr/lib64/libnvidia-egl-wayland.so.1.0.0
> /usr/share/doc/egl-wayland
> /usr/share/doc/egl-wayland/README.md
> /usr/share/egl/egl_external_platform.d/10_nvidia_wayland.json

> Makes sense.

Wouldn't libglvnd need to be changed before this can be reviewed as the directory /usr/share/egl/egl_external_platform.d/ would be unowned? 


Once Kwizart, Hans and you decide where you want the files I will update the spec file. :-)

Comment 23 Simone Caronni 2017-02-02 08:27:03 UTC
(In reply to leigh scott from comment #22)
> Wouldn't libglvnd need to be changed before this can be reviewed as the
> directory /usr/share/egl/egl_external_platform.d/ would be unowned?

Yes :)

> Once Kwizart, Hans and you decide where you want the files I will update the
> spec file. :-)

Thank you very much. Will review immediately after but it's already ok :)

Comment 24 Hans de Goede 2017-02-02 09:59:05 UTC
Ok,

So given the above discussion lets go with putting the */egl/egl_external_platform.d/ directories in the libglvnd-egl package. And make egl-wayland only have a main pkg I agree that that is cleaner, everyone ok with that ?

I need to do a libglvnd update today anyways (to fix an issue with some steam games) and I'll add them there then.

Leigh, can you respin one more time dropping the -common and putting all the files from -libs simply in the main package please ?

Note we can proceed without the libglvnd changes, things will work fine even if the directory is unowned until the libglvnd update lands. Worse case potential issue is the directories being left behind if someone installs + removes egl-wayland before the libglvnd changes land, which is a non issue really.

Regards,

Hans

Comment 25 Simone Caronni 2017-02-02 10:49:10 UTC
(In reply to Hans de Goede from comment #24)
> Note we can proceed without the libglvnd changes, things will work fine even
> if the directory is unowned until the libglvnd update lands. Worse case
> potential issue is the directories being left behind if someone installs +
> removes egl-wayland before the libglvnd changes land, which is a non issue
> really.

Actually we should need "Requires: libglvnd-egl" in the spec file.

@leigh, please wait to push the update until libglvnd updates land, so Taskotron will not complain with missing dependencies. Or just edit the libglvnd update adding egl-wayland in Bodhi.

Comment 26 Hans de Goede 2017-02-02 12:03:15 UTC
(In reply to Simone Caronni from comment #25)
> (In reply to Hans de Goede from comment #24)
> > Note we can proceed without the libglvnd changes, things will work fine even
> > if the directory is unowned until the libglvnd update lands. Worse case
> > potential issue is the directories being left behind if someone installs +
> > removes egl-wayland before the libglvnd changes land, which is a non issue
> > really.
> 
> Actually we should need "Requires: libglvnd-egl" in the spec file.

Since there is a BuildRequires libEGL-devel in the spec I was expecting there to be an autogenerated dependency on libEGL.so.0, but you're right that is not there, so we are going to need an explicit "Requires: libglvnd-egl" in the spec file.

Comment 27 Nicolas Chauvet (kwizart) 2017-02-02 12:27:23 UTC
I'm not sure if we aren't going too fast here.


As I understand egl-wayland, it's "aimed" to be generic implementation on top of (generic) EGL.
Unfortuntately, this doesn't seem to be completely generic yet as the produced shared object name suggest. (nvidia-egl-wayland.so.1).
Looking at the code:
1/ it seems to rely on a special magic number for a fifo dedicated to nvidia client (driver) side communication.
https://github.com/NVIDIA/egl-wayland/blob/master/wayland-egl/wayland-egl-ext.h#L33
2/ It seems to use on #ifdef EGL_NV_stream_remote available
https://www.khronos.org/registry/EGL/extensions/NV/EGL_NV_stream_remote.txt

Right now this eglext.h header update is available in mesa master (not in 17.0 or 13.0 branches) a98b3a0872f9c542e6db75d17b7875a3f0374a14

So right now we cannot build a full featured (nvidia-)egl-wayland replacing the pre-build version. (we would need to backport at least the header update in our mesa version).

Then I agree we don't know if this project will handle others (mesa?, amdgpu-pro ?) egl-wayland implementation or if it will only be the nvidia dedicated implementation. 
My bet is that it will became generic as soon as there are others adopters.
Hence I don't think moving the directories into libglvnd-gl is relevant for the long run. But I agree this is a possible way forward.

(don't forget arched requires: libglvnd-egl%{?_isa} )

Comment 28 Hans de Goede 2017-02-02 12:30:36 UTC
(In reply to Nicolas Chauvet (kwizart) from comment #27)
> I'm not sure if we aren't going too fast here.

This implementation might be nvidia specific, but the spec is generic, so the config dir should be generic too and if it turns out to be a nvidia only config dir we can always move dir ownership to another package
(e.g. this pkg) later having 2 pkgs own the same dir (in a transition period) is not a problem.

Comment 29 Hans de Goede 2017-02-02 12:33:16 UTC
The point about updating eglext.h is valid. I believe it is best to just add a copy of a new enough version fo that file to the pkg for now (if it is dropped in the include dir it should get used).

Comment 30 Nicolas Chauvet (kwizart) 2017-02-02 13:18:55 UTC
There is also a need to add egl-wayland to multilib whitelist
https://pagure.io/pungi-fedora/blob/master/f/fedora.conf#_178

Comment 31 Hans de Goede 2017-02-02 13:30:52 UTC
(In reply to Nicolas Chauvet (kwizart) from comment #30)
> There is also a need to add egl-wayland to multilib whitelist
> https://pagure.io/pungi-fedora/blob/master/f/fedora.conf#_178

Ack.

Comment 33 Hans de Goede 2017-02-02 15:24:13 UTC
Looks good. 2 remarks:

1) What about the remark about the eglext.h in mesa-devel being to old Nicolas made ?
2) Nitpick: "rm -rf $RPM_BUILD_ROOT" that has not been necessary for a long time now and should be removed

Comment 34 leigh scott 2017-02-02 17:40:30 UTC
(In reply to Hans de Goede from comment #33)
> Looks good. 2 remarks:
> 
> 1) What about the remark about the eglext.h in mesa-devel being to old
> Nicolas made ?

Is it possible to update the mesa egl headers

> 2) Nitpick: "rm -rf $RPM_BUILD_ROOT" that has not been necessary for a long
> time now and should be removed

That was inherited for rpmdev-newspec, I will remove when I import.

Comment 35 leigh scott 2017-02-02 17:42:11 UTC
(In reply to Hans de Goede from comment #33)
> Looks good. 2 remarks:
> 
> 1) What about the remark about the eglext.h in mesa-devel being to old
> Nicolas made ?

Or add them to libglvnd-*-devel

Comment 36 Hans de Goede 2017-02-02 18:01:43 UTC
(In reply to leigh scott from comment #34)
> (In reply to Hans de Goede from comment #33)
> > Looks good. 2 remarks:
> > 
> > 1) What about the remark about the eglext.h in mesa-devel being to old
> > Nicolas made ?
> 
> Is it possible to update the mesa egl headers

Yeah that is probably the best as other pkgs may need the new defines too. So I've just done this a new mesa with updated eglext.h is now building for F25+. I'll add it it to the libglvnd update in bodhi once the build is done.

Note this means you will need to do a buildroot override for mesa-13.0.3-7 for F25, unless that update goes stable before you import + build.

Comment 37 leigh scott 2017-02-02 18:08:56 UTC
(In reply to Hans de Goede from comment #36)
> (In reply to leigh scott from comment #34)
> > (In reply to Hans de Goede from comment #33)
> > > Looks good. 2 remarks:
> > > 
> > > 1) What about the remark about the eglext.h in mesa-devel being to old
> > > Nicolas made ?
> > 
> > Is it possible to update the mesa egl headers
> 
> Yeah that is probably the best as other pkgs may need the new defines too.
> So I've just done this a new mesa with updated eglext.h is now building for
> F25+. I'll add it it to the libglvnd update in bodhi once the build is done.
> 
> Note this means you will need to do a buildroot override for mesa-13.0.3-7
> for F25, unless that update goes stable before you import + build.

I think you will need egl.h and eglplatform.h from kronos as well

Comment 39 Hans de Goede 2017-02-02 18:39:13 UTC
(In reply to leigh scott from comment #38)
> see what I mean?
> https://cgit.freedesktop.org/mesa/mesa/commit/include/
> EGL?id=a98b3a0872f9c542e6db75d17b7875a3f0374a14

Yes, I've already fixed this (I've folded the new EGL_CAST macro into the updated eglext.h, no changes needed to egl.h then).

Comment 40 leigh scott 2017-02-02 19:26:01 UTC
It builds against mesa-*-17.0.0-0.4.rc2.fc26.x86_64 ok, I have also updated the spec and srpm in https://bugzilla.redhat.com/show_bug.cgi?id=1415143#c32 to make the build verbose.

Comment 41 Simone Caronni 2017-02-03 08:08:17 UTC
Package is fine, I don't see any issue.

You might want to use %{buildroot} instead of $RPM_BUILD_ROOT for consistency, but this is up to you. If you're using rpmdeve-newspec, you might want to generate it with macros instead (rpmdev-newspec -m).

Just remember to remove the "rm -rf $RPM_BUILD_ROOT" line before committing.

Package approved.

Comment 42 Gwyn Ciesla 2017-02-03 13:10:38 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/egl-wayland

Comment 43 leigh scott 2017-02-03 13:36:37 UTC
(In reply to Simone Caronni from comment #41)
> Package is fine, I don't see any issue.
> 
> You might want to use %{buildroot} instead of $RPM_BUILD_ROOT for
> consistency, but this is up to you. If you're using rpmdeve-newspec, you
> might want to generate it with macros instead (rpmdev-newspec -m).
> 
> Just remember to remove the "rm -rf $RPM_BUILD_ROOT" line before committing.
> 
> Package approved.

Thank you.

I have filed an upstream issue for the missing license file.

https://github.com/NVIDIA/egl-wayland/pull/1

Comment 44 leigh scott 2017-02-03 14:03:42 UTC
(In reply to Hans de Goede from comment #36)
> (In reply to leigh scott from comment #34)
> > (In reply to Hans de Goede from comment #33)
> > > Looks good. 2 remarks:
> > > 
> > > 1) What about the remark about the eglext.h in mesa-devel being to old
> > > Nicolas made ?
> > 
> > Is it possible to update the mesa egl headers
> 
> Yeah that is probably the best as other pkgs may need the new defines too.
> So I've just done this a new mesa with updated eglext.h is now building for
> F25+. I'll add it it to the libglvnd update in bodhi once the build is done.

Should egl-wayland be added to the f25 libglvnd update in bodhi or on it's own?

https://koji.fedoraproject.org/koji/buildinfo?buildID=838167

Comment 45 Simone Caronni 2017-02-06 08:31:09 UTC
(In reply to leigh scott from comment #44)
> Should egl-wayland be added to the f25 libglvnd update in bodhi or on it's
> own?
> 
> https://koji.fedoraproject.org/koji/buildinfo?buildID=838167

Would make sense to add, so dependencies are satisfied and taskotron would not complain.

But instead I would say wait a bit and create the update on its own. There is already too many discussions on the libglvnd update, better not throwing additional fuel on it.

Comment 46 Hans de Goede 2017-02-06 08:53:53 UTC
Hi,

(In reply to Simone Caronni from comment #45)
> (In reply to leigh scott from comment #44)
> > Should egl-wayland be added to the f25 libglvnd update in bodhi or on it's
> > own?
> > 
> > https://koji.fedoraproject.org/koji/buildinfo?buildID=838167
> 
> Would make sense to add, so dependencies are satisfied and taskotron would
> not complain.
> 
> But instead I would say wait a bit and create the update on its own. There
> is already too many discussions on the libglvnd update, better not throwing
> additional fuel on it.

Ack, that seems best to me too.

Regards,

Hans


Note You need to log in before you can comment on or make changes to this bug.