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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ijg-libjpeg] add new port #14583

Merged
merged 11 commits into from Dec 28, 2020
Merged

[ijg-libjpeg] add new port #14583

merged 11 commits into from Dec 28, 2020

Conversation

luncliff
Copy link
Contributor

@luncliff luncliff commented Nov 16, 2020

Changes

Which triplets are supported/not supported? Have you updated the CI baseline?

Build status of the fork - https://dev.azure.com/luncliff/personal/_build?definitionId=47&_a=summary

  • Pass
    • x64-uwp
    • x64-windows
    • x64-windows-static
    • x86-windows
    • x64-osx
    • x64-linux (requires tool dos2unix)
  • Fail
    • s390x-linux
    • wasm32-emscripten

Does your PR follow the maintainer guide?

Yes 馃憤

Works In Progress

  • Test available triplets and update "supports"
  • jconfig.h generation with configure in the release (Linux)
  • Feedback for the following Issues

Issues

1. The port uses date for its version-string (solved)

Its archive is named jpegsr9d.zip. README specifies release 9d of 12-Jan-2020.
I'm confused about this case, so applied 2020-01-12 temporarily.

Can version-string be 9d in this case? (like tbb's 2020_U3...?)

2. The port defines its own CMakeLists.txt (solved)

The official release(http://www.ijg.org/files/jpegsr9d.zip) already contains configure for the UNIX machine, but nothing for the Windows environment. The file is for the case.

Does it right to add CMakeLists.txt in this case?
or should this port behave differently like openssl-unix, openssl-windows?

3. The port's output conflicts with 'libjpeg-turbo' (solved)

The port creates jpeg.lib, jpeglib.h, etc. Which can be installed via libjpeg-turbo.

Do I have to skip the build in this ci.baseline.txt?

luncliff and others added 3 commits November 13, 2020 18:40
* Specify _CRT_SECURE_NO_WARNINGS for Windows
* manifest: disable "supports" of emscripten, wasm32
* remove WINDOWS_EXPORT_ALL_SYMBOLS to follow the guideline
@luncliff luncliff marked this pull request as draft November 16, 2020 01:58
@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 16, 2020
@NancyLi1013
Copy link
Contributor

Hi @luncliff

Thanks for your PR.

Since this PR is still in progress, I will answer the questions that you mentioned above now.

For issue 1, if 9d is the version for ijp-libjpeg, of course, we can use it as Version-String here. Otherwise, a date might be one choice. If using a date, it should be the one that you create this PR. Such as 2020-11-16.

Please refer to this doc https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/control-files.md#version

For issue 2, it is possible to add CMakeLists.txt for the port. We recommend to use CMake to build the ports.

https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#prefer-using-cmake

If the source codes have no difference between Windows and Unix platform, there is no need to judge the platforms.

For issue 3, it is necessary to skip the build in ci.baseline.txt since there are conflicts between these two ports.

Please refer to this

# Conflicts with libjpeg-turbo
mozjpeg:arm64-windows      = skip
mozjpeg:arm-uwp            = skip
mozjpeg:x64-linux          = skip
mozjpeg:x64-osx            = skip
mozjpeg:x64-uwp            = skip
mozjpeg:x64-windows        = skip
mozjpeg:x64-windows-static = skip
mozjpeg:x86-windows        = skip

Note: It would be better to add the messages to remind the users in portfile.cmake like this:

https://github.com/microsoft/vcpkg/blob/master/ports/libressl/portfile.cmake

ports/ijg-libjpeg/vcpkg.json Outdated Show resolved Hide resolved
scripts/ci.baseline.txt Outdated Show resolved Hide resolved
ports/ijg-libjpeg/CMakeLists.txt Show resolved Hide resolved
ports/ijg-libjpeg/CMakeLists.txt Outdated Show resolved Hide resolved
ports/ijg-libjpeg/CMakeLists.txt Show resolved Hide resolved
ports/ijg-libjpeg/portfile.cmake Outdated Show resolved Hide resolved
@luncliff luncliff marked this pull request as ready for review November 18, 2020 10:26
ports/ijg-libjpeg/vcpkg.json Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

@luncliff

Have you tested this port on your local? Does it work fine?

@luncliff
Copy link
Contributor Author

@NancyLi1013

@luncliff

Have you tested this port on your local? Does it work fine?

Sory for the point. Not all triplets are tested. I will test them with in a days.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@luncliff
Copy link
Contributor Author

@NancyLi1013 I found generated jconfig.h in x64-linux isn't usable. The others are fine. I will report when I find the correct fix.

Followed the way of  https://github.com/LuaDist/libjpeg
Place cmakedefine in jconfig.txt and use it as a template.
@luncliff
Copy link
Contributor Author

luncliff commented Dec 6, 2020

@NancyLi1013 . I followed the way of LuaDist/libjpeg and now the generated jconfig.h works fine.

@NancyLi1013
Copy link
Contributor

Thanks for your test and feedback. I noticed that this port requires tool dos2unix to build on Linux. If so, we'd better add some messages like this in portfile.cmake:

if(VCPKG_TARGET_IS_LINUX)
    message(FATAL_ERROR "${PORT} requires `dos2unix,` this can be installed on Ubuntu systems via apt-get install dos2unix.")
endif()

@luncliff
Copy link
Contributor Author

luncliff commented Dec 7, 2020

@NancyLi1013, the commit 2a1eb90 removed the dependency.
The tool was required to remove CRLF in the configure scripts, but now the port use CMake configure_file instead of the script execution. :)

ports/ijg-libjpeg/portfile.cmake Outdated Show resolved Hide resolved
ports/ijg-libjpeg/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

@luncliff

Sorry for the delay. If you have tested these triplets on your local and all these triplets can build fine, there is nothing to be done for this PR now.

@luncliff
Copy link
Contributor Author

Sorry @NancyLi1013 , I forgot to mention you after the last update.

Build logs for the 840bea4 are here.
The job installs dos2unix but it isn't used anymore.

Wish it looks good to you. 馃巹 馃巺

@NancyLi1013
Copy link
Contributor

Thanks for your update. The build results look good.

But I didn't notice the build info for arm on Windows, such as arm-uwp and arm64-windows triplets.

Have you tested these two triplets on Windows?

@luncliff
Copy link
Contributor Author

@NancyLi1013 , I just added arm64-windows and arm-uwp to the configuration.
Fortunately it passes. The log is at the end of "install libjpeg(major triplets)" of 'vs2017-win2016'.

@NancyLi1013
Copy link
Contributor

LGTM now, thanks for your PR @luncliff.

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Dec 28, 2020
@BillyONeal BillyONeal merged commit bba1522 into microsoft:master Dec 28, 2020
@BillyONeal
Copy link
Member

Thanks :)

ryukw7 pushed a commit to ryukw7/vcpkg that referenced this pull request Dec 29, 2020
@luncliff luncliff deleted the port/libjpeg branch January 1, 2021 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants