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

openjp2: cleanup obsolete components #215

Merged

Conversation

dabrain34
Copy link
Contributor

@dabrain34 dabrain34 commented Oct 25, 2021

improve bin dependency checking

png, libtiff, lcms2 dep checking
can be conditional.
Remove zlib dependency as nobody is using it.


Remove unmaintained JPWL, JP3D and MJ2

Since uclouvain/openjpeg#1350
the components JPWL, JP3d and MJ2 will not be
maintained in 2.5.0.

As win32 build was facing an issue with mj2 build and lrintf inline,
(see uclouvain/openjpeg#1339), better
to remove these useles components who will be removed in v2.5.0.

@dabrain34 dabrain34 force-pushed the dab_opj2_cleanup_obsolete_components branch from 5ad9474 to c482bad Compare October 25, 2021 11:14
@dabrain34
Copy link
Contributor Author

@xclaesse ^

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

You seem to have accidentally resolved a merge conflict by adding a meson.build~HEAD file

subprojects/packagefiles/libopenjp2/src/lib/meson.build Outdated Show resolved Hide resolved
zlib_dep = dependency('zlib')

libpng_dep = dependency('libpng')
libpng_dep = dependency('libpng', required : false)
Copy link
Member

Choose a reason for hiding this comment

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

Ideally these would be options of some sort to avoid "automagic dependencies". What does upstream do, hmmm...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used this as it is disabled by config if the dep is not found.
See

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested on VS2019 prompt without any wrap

Copy link
Member

Choose a reason for hiding this comment

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

I think you misunderstood.

I'm referring to this: https://wiki.gentoo.org/wiki/Project:Quality_Assurance/Automagic_dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But seems that this libpng (or libtiff) dependency is only necessary for the bins opj_* and the lib support can be optional for these tools. Any recommandation to address your concern here ?

Copy link
Member

Choose a reason for hiding this comment

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

The classic method is to add a feature option to meson_options.txt, and use that as the required: get_option('...') kwarg.

That configure option would then allow consumers to select whether they want to ensure the bins are/are not built with libpng support, or have meson automatically build with libpng support if it can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is build_codec_with_png_support suitable to you ?

Copy link
Member

Choose a reason for hiding this comment

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

That is a very long name which I think people probably don't want to pass as a command line option. How about libpng? :p Or codec_libpng or app_libpng.

TBH some of the option names in there aren't very good names, e.g. all the ones starting with opj_ are already project-scoped by definition and meson does subproject name prefixing. The "codec" option is a bit odd since there is no longer any other executable group to confuse the issue if it was called "build_apps".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree to change the option name but i'm not a big fan adding a libpng option. I prefer to require it if the app option is enabled.

Copy link
Contributor Author

@dabrain34 dabrain34 Oct 29, 2021

Choose a reason for hiding this comment

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

Regarding the opj_ prefix I kept it to follow the cmake files. I would keep it as it's option for openjp2 specific subfolder

@dabrain34 dabrain34 force-pushed the dab_opj2_cleanup_obsolete_components branch 2 times, most recently from e3decba to 5c8e5d1 Compare October 29, 2021 12:12
@jpakkane
Copy link
Member

jpakkane commented Nov 8, 2021

LGMT. Anyone else?

@eli-schwartz
Copy link
Member

I had previously mentioned that enabling additional functionality (png, tiff, lcms2) for the command line programs using required: false is automagic dependencies and I'm not entirely sure that is a good idea compared to using a feature option.

@neheb
Copy link
Collaborator

neheb commented Sep 2, 2022

Needs rebasing.

Since uclouvain/openjpeg#1350
the components JPWL, JP3d and MJ2 will not be
maintained in 2.5.0.

As win32 build was facing an issue with mj2 build and lrintf inline,
(see  uclouvain/openjpeg#1339), better
to remove these useles components who will be removed in v2.5.0.
@dabrain34 dabrain34 force-pushed the dab_opj2_cleanup_obsolete_components branch from 5c8e5d1 to 7ef9651 Compare September 5, 2022 13:34
@eli-schwartz
Copy link
Member

What about adding feature options for the optional dependencies?

@dabrain34 dabrain34 force-pushed the dab_opj2_cleanup_obsolete_components branch from 7ef9651 to 5b3b1db Compare September 8, 2022 07:54
@dabrain34
Copy link
Contributor Author

Thanks Eli for your recommendations. I hope it will fit your expectations this time.

@jpakkane
Copy link
Member

Still looks good. Anyone else have comments?

Stéphane Cerveau added 3 commits September 16, 2022 10:32
png, libtiff, lcms2 dep checking
can be conditional.
Remove zlib dependency as nobody is using it.
Remove option for pkg-config files generation
Rename to build_codec_apps to make it clearer
@dabrain34 dabrain34 force-pushed the dab_opj2_cleanup_obsolete_components branch from 5b3b1db to 70c8add Compare September 16, 2022 08:33
@jpakkane
Copy link
Member

Please ack, I don't want to merge things marked as blocked.

@eli-schwartz eli-schwartz dismissed their stale review September 16, 2022 20:38

Feedback has been acted on.

@eli-schwartz
Copy link
Member

I personally don't love the booleans instead of feature options, but that's a personal preference.

Feel free to merge it if you like, I myself would rather wait to make up my mind over the weekend but that's not a merge blocker anymore.

@jpakkane
Copy link
Member

Incremental improvements can always be done later. Thanks.

@jpakkane jpakkane merged commit 4814578 into mesonbuild:master Sep 16, 2022
@xclaesse
Copy link
Member

This PR is missing a new entry in releases.json, so it did not generate a new release.

dabrain34 added a commit to dabrain34/wrapdb that referenced this pull request Sep 21, 2022
Following the lastest change
mesonbuild#215
a new release can now be built.
xclaesse pushed a commit that referenced this pull request Sep 21, 2022
Following the lastest change
#215
a new release can now be built.
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.

5 participants