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

wepwebkit-depends: check commercial license #521

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

kwavnet
Copy link
Contributor

@kwavnet kwavnet commented Nov 25, 2024

acceptance properly for openh264 and gstreamer1.0-libav. For both LICENSE_FLAGS_ACCEPTED needs to
contain "commercial" to use it

@kwavnet kwavnet force-pushed the fix/commercial-license branch from 1a724ad to 8d5043b Compare November 25, 2024 11:20
@psaavedra psaavedra requested a review from clopez December 3, 2024 11:50
@kwavnet kwavnet force-pushed the fix/commercial-license branch from 8d5043b to 55d7610 Compare December 5, 2024 07:14
acceptance properly for openh264 and gstreamer1.0-libav.
For both LICENSE_FLAGS_ACCEPTED needs to
contain "commercial" to use it

Signed-off-by: Konrad Weihmann <[email protected]>
@kwavnet kwavnet force-pushed the fix/commercial-license branch from 55d7610 to 9f90e14 Compare December 6, 2024 06:29
@kwavnet
Copy link
Contributor Author

kwavnet commented Dec 6, 2024

@psaavedra the walnascar error looks unrelated - my hot guess would be https://git.yoctoproject.org/poky/commit/meta/recipes-support/icu?id=315588665f9a15f130b770c7a69ecfc37a9edf9d - there has been some changelog regarding UnicodeString in ICU 76

@psaavedra
Copy link
Member

@psaavedra the walnascar error looks unrelated - my hot guess would be https://git.yoctoproject.org/poky/commit/meta/recipes-support/icu?id=315588665f9a15f130b770c7a69ecfc37a9edf9d - there has been some changelog regarding UnicodeString in ICU 76

Yes, like in #522 (comment) .

I've created #523 for addressing this separately.

@psaavedra
Copy link
Member

@clopez can you r? this change. Thanks.

conf/layer.conf Outdated
webserver \
"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is not right, meta-webserver is part of meta-openembedded and were are not specifiying there all the other meta-openembedded sublayers that we depend on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RDEPENDS:packagegroup-wpewebkit-depends-tests adds apache2 (at packagegroup-wpewebkit-depends.bb)

Without that patch, a world build will fail (or any other explicit build that includes packagegroup-wpewebkit-depends)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so either that should be removed (as it looks rather test specific) or the meta-webserver will need to be mandatory

Copy link
Member

Choose a reason for hiding this comment

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

It is added for the execution of the layout tests. The question is whether the image is being used for this or not in the WebKit CI (or is expected).

Copy link
Member

@psaavedra psaavedra Dec 9, 2024

Choose a reason for hiding this comment

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

I see both point of views. I think the observation that @kwavnet is right however the proposal is too much restrictive and forces to meta-webkit a dependency that is only required for development what is not ideal for other case of uses (more common cases).

I think we are OK with the 3bfa477 change. So, first of all I'd suggest to split the proposal into two PRs:

  • One exclusively for the changes related to licenses.
  • Another for implementing a dynamic logic to handle meta-webserver and other dependencies with other layers.

then, this is my proposal for the second commit:

To address the issue of handling optional dependencies like meta-webserver and avoiding forcing its inclusion in bblayers.conf as a hard dependency, I propose implementing an approach based on dynamic layers. This would ensure that features related to web servers, such as Apache, are activated only if the meta-webserver layer is available:

Dynamic Layer Logic:

This proposal has the following benefits:

  • Avoids requiring users to add meta-webserver to bblayers.conf if they don’t need it.
  • Keeps the layer more modular and reduces unnecessary strict dependencies.
  • We could do a similar change for other optional layers like meta-multimedia.

Finally I think it is also important a clear documentation about this behavior in the README files or related commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, from my point of view.
meta-openembedded/meta-oe is mandatory already, hence adding another layer checked out from the same repository doesn't hurt right now.

But what I totally agree on, is that the entire packagegroup-wpewebkit-depends.bb file needs to be rethought.
Too much stuff is cramped in there (and I can't say from my limited point of view what each of these dependencies are for).

So I would like to make a counter proposal.

Everything related to tests is moved to a sublayer e.g. meta-webkit-test, that sets those requirements.
That layer can be added to the mix (e.g. here in CI) if needed, but for consumers (such as myself) it remains optional.

An example of that is https://github.com/meta-flutter/meta-flutter and the sublayer https://github.com/meta-flutter/meta-flutter/tree/scarthgap/meta-flutter-apps.

My go to approach would be to merge this here and do the split in the next step.
I'm happy to help but I would need some input what out of the monolithic packagegroup-wpewebkit-depends.bb is truly optional, resp. what is test related

Copy link
Contributor

@clopez clopez Dec 9, 2024

Choose a reason for hiding this comment

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

The problem with merging this is that we are forcing every user to depend on meta-webserver even when they don't need it. So I would rather don't do that.

I suggest that you split this PR in two: The change for the licenses is fine, but the other change needs to be re-implemented in a way that we don't force every user of this layer to depend on meta-webserver.

And I prefer the solution suggested by @psaavedra rather than splitting the repository in two. Having to maintain yet another repository adds more burden to the maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, commit removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! merged. Feel free to send another PR for the other change if you wish.

Copy link
Member

@psaavedra psaavedra Dec 9, 2024

Choose a reason for hiding this comment

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

Another solution it could be to use LAYERRECOMMENDS
(LAYERRECOMMENDS_meta-webkit = "meta-webserver meta-multimedia" ) in combination with anonymous functions like:

python () {
    if d.getVar('BBLAYERS').find('meta-webserver') != -1:
        d.appendVar('RDEPENDS:packagegroup-wpewebkit', ' apache2')
}

@kwavnet kwavnet force-pushed the fix/commercial-license branch 2 times, most recently from 19673ca to 3bfa477 Compare December 9, 2024 16:22
@clopez clopez merged commit 8501a53 into Igalia:main Dec 9, 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.

3 participants