Bug 1421055 (deepin-cogl) - Review Request: deepin-cogl - An object oriented GL/GLES Utility Layer for Deepin
Summary: Review Request: deepin-cogl - An object oriented GL/GLES Utility Layer for De...
Keywords:
Status: CLOSED ERRATA
Alias: deepin-cogl
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Robin Lee
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: NotReady
Depends On:
Blocks: DeepinDEPackageReview deepin-mutter
TreeView+ depends on / blocked
 
Reported: 2017-02-10 09:09 UTC by sensor.wen
Modified: 2018-01-01 01:45 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-10-09 19:58:53 UTC
Type: ---
robinlee.sysu: fedora-review?


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
GNOME Bugzilla 787443 None None None 2019-03-21 13:58:34 UTC

Comment 1 Robin Lee 2017-08-06 00:16:35 UTC
This package should not conflict with the original cogl.

Comment 2 Robert-André Mauchin 🐧 2017-08-27 19:01:44 UTC
Is it that bad if it conflicts with the original cogl? It provides the same functions, with two additions for deepin.

Comment 3 sensor.wen 2017-08-31 16:07:52 UTC
SPEC:  https://copr-be.cloud.fedoraproject.org/results/mosquito/deepin/fedora-26-x86_64/00596540-deepin-cogl/deepin-cogl.spec
SRPM:  https://copr-be.cloud.fedoraproject.org/results/mosquito/deepin/fedora-26-x86_64/00596540-deepin-cogl/deepin-cogl-1.22.5-2.fc26.src.rpm

- remove the 'conflicts' and 'obsoletes' tag

It's mostly the same as the original cogl. I don't want anyone who doesn't use deepin to update it. So what should i do with it?

Comment 4 Robin Lee 2017-08-31 16:31:19 UTC
Since version of deepin-cogl is greater than cogl, anybody runs 'dnf install cogl' will actually install deepin-cogl. Also applies to pungi.

Comment 5 Robert-André Mauchin 🐧 2017-09-01 05:46:31 UTC
@mosquito: I've discussed this with other packagers on IRC and the conclusion was it's best to rename it in order to avoid any conflict with the original cogl.

Comment 6 sensor.wen 2017-09-01 07:11:47 UTC
Upstream has not free time for this. I think the best way is use another path to save libraries and header files.

Comment 7 Zbigniew Jędrzejewski-Szmek 2017-09-02 10:38:21 UTC
Is this package really needed? The diff between gnome's cogl 1.22 and this fork is just a few commits. Wouldn't it be possible to cooperate with Fedora cogl maintainers and cogl upstream to upstream those patches and dispose of the fork:

Only in deepin-cogl cogl-1.22 branch:
* 1e1e1b8a97 winsys: Disable sync_control
* f28fe60db1 Add LICENSE
* 9ee8ef2d2d (tag: 1.22.3) Change debian dir
* a34f7835c0 Debian dir migration
* 9e113c10da Add debian dir
* d8b34ab060 texture: Support copy_sub_image
* 78636289b0 Add GL_ARB_shader_texture_lod support

Only in upstream cogl cogl-1.22 branch:
* cbdde65b7e (upstream/cogl-1.22) Add Nepali translation
* 84d9ed33ef Update Malayalam translation
* 811f285a8e Update po/Makevars
* 3baa2d7a65 Updated Norwegian bokmål translation.
* 0bc94d13df Update Friulian translation
* cdb3229f54 Update Friulian translation
* df29d85990 Add Friulian translation
* b583e21d86 Fix an incorrect preprocessor conditional

@pbrobinson, what do you think?

Comment 8 Zamir SUN 2017-09-02 12:18:01 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> * 9ee8ef2d2d (tag: 1.22.3) Change debian dir
> * a34f7835c0 Debian dir migration
> * 9e113c10da Add debian dir

@mosquito, can you try how this works if we manually create the dir as the commit above?

> @pbrobinson, what do you think?

I see Peter is not copied into the bug, so adding him to cc.

Comment 9 Peter Robinson 2017-09-04 11:34:16 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> Is this package really needed? The diff between gnome's cogl 1.22 and this
> fork is just a few commits. Wouldn't it be possible to cooperate with Fedora
> cogl maintainers and cogl upstream to upstream those patches and dispose of
> the fork:
> 
> Only in deepin-cogl cogl-1.22 branch:
> * 1e1e1b8a97 winsys: Disable sync_control
> * f28fe60db1 Add LICENSE
> * 9ee8ef2d2d (tag: 1.22.3) Change debian dir
> * a34f7835c0 Debian dir migration
> * 9e113c10da Add debian dir
> * d8b34ab060 texture: Support copy_sub_image
> * 78636289b0 Add GL_ARB_shader_texture_lod support
> 
> Only in upstream cogl cogl-1.22 branch:
> * cbdde65b7e (upstream/cogl-1.22) Add Nepali translation
> * 84d9ed33ef Update Malayalam translation
> * 811f285a8e Update po/Makevars
> * 3baa2d7a65 Updated Norwegian bokmål translation.
> * 0bc94d13df Update Friulian translation
> * cdb3229f54 Update Friulian translation
> * df29d85990 Add Friulian translation
> * b583e21d86 Fix an incorrect preprocessor conditional
> 
> @pbrobinson, what do you think?

Yes, absolutely, having overlapping libraries is ridiculous.

Also looking at the 7 patches in deepin-cogl listed above there's probably only two that actually would affect Fedora, the top 5 listed commits are debian/windows/licence related, it's likely only the bottom two of real effect to Fedora and those should go for review upstream (well they all should).

Comment 10 sensor.wen 2017-09-05 15:02:02 UTC
Only the following two patches are required for Cogl.

https://github.com/linuxdeepin/deepin-cogl/commit/78636289b073d67209a20145ef0dc003f2d77db6    Add GL_ARB_shader_texture_lod support
https://github.com/linuxdeepin/deepin-cogl/commit/d8b34ab0604d80d0be22b8b78e9aa6bf4fac7db0    texture: Support copy_sub_image

and then the `clutter` needs following patch.

https://github.com/sonald/clutter/commit/0e6e542a05e071791ca625d08f8efa8111a347db     Support actors(blur) to be always redrawn

I asked the developer of Deepin why didn't submit these patches to upstream, and he said: "upstream does not merge the patches they doesn't need use".
@Peter Robinson could you help me?

Comment 11 Peter Robinson 2017-09-05 17:06:09 UTC
(In reply to sensor.wen from comment #10)
> Only the following two patches are required for Cogl.
> 
> https://github.com/linuxdeepin/deepin-cogl/commit/
> 78636289b073d67209a20145ef0dc003f2d77db6    Add GL_ARB_shader_texture_lod
> support
> https://github.com/linuxdeepin/deepin-cogl/commit/
> d8b34ab0604d80d0be22b8b78e9aa6bf4fac7db0    texture: Support copy_sub_image
> 
> and then the `clutter` needs following patch.
> 
> https://github.com/sonald/clutter/commit/
> 0e6e542a05e071791ca625d08f8efa8111a347db     Support actors(blur) to be
> always redrawn

We need upstream bugs for both (cogl/clutter) and then link those to downstream bugs against the appropriate Fedora components, close off this request as it's not the place to deal with it.

> I asked the developer of Deepin why didn't submit these patches to upstream,
> and he said: "upstream does not merge the patches they doesn't need use".
> @Peter Robinson could you help me?

I don't believe that to be true, and it's certainly not been my experience.

Comment 12 sensor.wen 2017-09-05 17:56:26 UTC
(In reply to Peter Robinson from comment #11)
> (In reply to sensor.wen from comment #10)
> > Only the following two patches are required for Cogl.
> > 
> > https://github.com/linuxdeepin/deepin-cogl/commit/
> > 78636289b073d67209a20145ef0dc003f2d77db6    Add GL_ARB_shader_texture_lod
> > support
> > https://github.com/linuxdeepin/deepin-cogl/commit/
> > d8b34ab0604d80d0be22b8b78e9aa6bf4fac7db0    texture: Support copy_sub_image
> > 
> > and then the `clutter` needs following patch.
> > 
> > https://github.com/sonald/clutter/commit/
> > 0e6e542a05e071791ca625d08f8efa8111a347db     Support actors(blur) to be
> > always redrawn
> 
> We need upstream bugs for both (cogl/clutter) and then link those to
> downstream bugs against the appropriate Fedora components, close off this
> request as it's not the place to deal with it.
> 
> > I asked the developer of Deepin why didn't submit these patches to upstream,
> > and he said: "upstream does not merge the patches they doesn't need use".
> > @Peter Robinson could you help me?
> 
> I don't believe that to be true, and it's certainly not been my experience.

Thanks @Peter Robinson.
I try to create new upstream bugs for both on Github.

Comment 13 Peter Robinson 2017-09-05 18:02:41 UTC
> Thanks @Peter Robinson.
> I try to create new upstream bugs for both on Github.

I suspect you'll need bugzilla.gnome.org rather than github

Comment 14 sensor.wen 2017-09-08 14:06:22 UTC
(In reply to Peter Robinson from comment #13)
> > Thanks @Peter Robinson.
> > I try to create new upstream bugs for both on Github.
> 
> I suspect you'll need bugzilla.gnome.org rather than github

https://bugzilla.gnome.org/show_bug.cgi?id=787443

I submitted new upstream bug for this. But I can't provide much information about patches. If you find out some problem, i will tell the developer.

Comment 15 sensor.wen 2017-09-23 17:57:10 UTC
Hi, @Peter

Cogl patches are approved on the upstream, but new versions are not available.  May be we need to attach them to cogl package.

https://bugzilla.gnome.org/show_bug.cgi?id=787443#c8

Comment 16 Zamir SUN 2017-09-24 01:48:45 UTC
(In reply to sensor.wen from comment #15)
> Hi, @Peter
> 
> Cogl patches are approved on the upstream, but new versions are not
> available.  May be we need to attach them to cogl package.
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=787443#c8

Bowen, 
See comment 11. I suggest file a bug asking the cogl maintainers to add them after the patches landed in cogl git repo.

Comment 17 Peter Robinson 2017-09-24 14:47:54 UTC
> Cogl patches are approved on the upstream, but new versions are not
> available.  May be we need to attach them to cogl package.

Saw that, I'm traveling and in meetings all week so I won't have time to look closely for a bit but if you could give me a PR [1] I can merge/push a build this week.

[1] https://src.fedoraproject.org/rpms/cogl

Comment 18 sensor.wen 2017-09-28 14:29:14 UTC
I submitted a new Pull Request for cogl. It looks good. @Peter

https://src.fedoraproject.org/rpms/cogl/pull-request/1

Comment 19 Peter Robinson 2017-09-28 16:22:09 UTC
It's FBTFS, looks like there's a new dependency.

https://koji.fedoraproject.org/koji/taskinfo?taskID=22126976

Comment 20 sensor.wen 2017-10-02 20:28:14 UTC
Added the pkgconfig(egl) BReq. Build passed. @Peter

PR: https://src.fedoraproject.org/rpms/cogl/pull-request/2

Test: https://koji.fedoraproject.org/koji/taskinfo?taskID=22219077

Comment 21 Fedora Update System 2017-10-03 09:02:15 UTC
cogl-1.22.2-7.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2017-1bad42b7b7

Comment 22 Fedora Update System 2017-10-06 04:25:23 UTC
cogl-1.22.2-7.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-1bad42b7b7

Comment 23 Fedora Update System 2017-10-09 19:58:53 UTC
cogl-1.22.2-7.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.


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