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

[libjuice] Add new port #13703

Merged
merged 12 commits into from
Oct 23, 2020
Merged

Conversation

Nemirtingas
Copy link
Contributor

@Nemirtingas Nemirtingas commented Sep 24, 2020

Describe the pull request

  • What does your PR fix? Fixes issue [New Port Request] libdatachannel #13487, I'm also creating a port for libdatachannel.

  • Which triplets are supported/not supported?
    I tested on lwindows x86, x64 static library, static CRT, it works.
    I tested on linux, windows x86, x64 static library, it works.
    I tested on linux, windows x86, x64 dynamic library, it works.

  • Have you updated the CI baseline? Hum, no ?

  • Does your PR follow the maintainer guide?
    The base CMake file builds 2 distinct libraries for static (which is not exported) and dynamic library. I've patched the CMakeLists.txt to build only the shared library and set it static if static library is picked in vcpkg.

This is my first port, please review carefully,

@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Sep 24, 2020
ports/libjuice/CONTROL Outdated Show resolved Hide resolved
ports/libjuice/CONTROL Outdated Show resolved Hide resolved
ports/libjuice/portfile.cmake Outdated Show resolved Hide resolved
ports/libjuice/portfile.cmake Outdated Show resolved Hide resolved
ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 changed the title libjuice port [libjuice] Add new port Sep 24, 2020
@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor Author

@Nemirtingas Nemirtingas left a comment

Choose a reason for hiding this comment

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

Changed in last version

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This overall looks like a really great PR; I just have a few more suggestions on how to reduce the size of the patch file. Minimizing the number of lines changed is really important to make upgrading in the future seamless :)

ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
ports/libjuice/fix-for-vcpkg.patch Outdated Show resolved Hide resolved
ports/libjuice/CONTROL Outdated Show resolved Hide resolved
scripts/ci.baseline.txt Outdated Show resolved Hide resolved
Nemirtingas and others added 2 commits October 9, 2020 11:09
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@NancyLi1013
Copy link
Contributor

The failures on x64-uwp and arm-uwp:

4>LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification [D:\buildtrees\libjuice\x64-uwp-dbg\juice.vcxproj]
            Creating library D:/buildtrees/libjuice/x64-uwp-dbg/Debug/juice.lib and object D:/buildtrees/libjuice/x64-uwp-dbg/Debug/juice.exp
     4>random.obj : error LNK2019: unresolved external symbol CryptAcquireContext referenced in function random_bytes [D:\buildtrees\libjuice\x64-uwp-dbg\juice.vcxproj]
     4>D:\buildtrees\libjuice\x64-uwp-dbg\Debug\juice.dll : fatal error LNK1120: 1 unresolved externals [D:\buildtrees\libjuice\x64-uwp-dbg\juice.vcxproj]
     4>Done Building Project "D:\buildtrees\libjuice\x64-uwp-dbg\juice.vcxproj" (default targets) -- FAILED.
     3>Done Building Project "D:\buildtrees\libjuice\x64-uwp-dbg\ALL_BUILD.vcxproj" (default targets) -- FAILED.
     1>Done Building Project "D:\buildtrees\libjuice\x64-uwp-dbg\install.vcxproj" (default targets) -- FAILED.

Could you please look into this and try to fix it?

@Nemirtingas
Copy link
Contributor Author

Nemirtingas commented Oct 13, 2020

random_bytes

Hi,
I'm asking the library owner if he wants to switch to the new crypto API.

@Nemirtingas
Copy link
Contributor Author

Looks to build now.

Copy link
Contributor Author

@Nemirtingas Nemirtingas left a comment

Choose a reason for hiding this comment

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

Fixed

@NancyLi1013
Copy link
Contributor

Thanks for your quick fix.
LGTM now.

Could you please help confirm if you have tested the feature nettle? Does it work fine? If you didn't test it, I will do this later.

@Nemirtingas
Copy link
Contributor Author

Nemirtingas commented Oct 14, 2020

Thanks for your quick fix.
LGTM now.

Could you please help confirm if you have tested the feature nettle? Does it work fine? If you didn't test it, I will do this later.

Nettle has issue when building on Linux.

cat /vcpkg_libjuice/buildtrees/gmp/autoconf-x64-linux-err.log
autoreconf: Entering directory `.'
autoreconf: configure.ac: not using Gettext
autoreconf: running: aclocal --force 
configure.ac:3782: warning: AC_LIBTOOL_PROG_COMPILER_PIC is m4_require'd but not m4_defun'd
acinclude.m4:2363: GMP_ASM_X86_GOT_UNDERSCORE is expanded from...
configure.ac:3782: the top level
configure.ac:3785: warning: AC_ENABLE_SHARED is m4_require'd but not m4_defun'd
acinclude.m4:2717: GMP_ASM_X86_MCOUNT is expanded from...
configure.ac:3785: the top level
configure.ac:3785: warning: AC_PROG_LIBTOOL is m4_require'd but not m4_defun'd
acinclude.m4:2717: GMP_ASM_X86_MCOUNT is expanded from...
configure.ac:3785: the top level
autoreconf: configure.ac: tracing
configure.ac:3782: warning: AC_LIBTOOL_PROG_COMPILER_PIC is m4_require'd but not m4_defun'd
acinclude.m4:2363: GMP_ASM_X86_GOT_UNDERSCORE is expanded from...
configure.ac:3782: the top level
configure.ac:3785: warning: AC_ENABLE_SHARED is m4_require'd but not m4_defun'd
acinclude.m4:2717: GMP_ASM_X86_MCOUNT is expanded from...
configure.ac:3785: the top level
configure.ac:3785: warning: AC_PROG_LIBTOOL is m4_require'd but not m4_defun'd
acinclude.m4:2717: GMP_ASM_X86_MCOUNT is expanded from...
configure.ac:3785: the top level
autoreconf: configure.ac: not using Libtool
autoreconf: running: /usr/bin/autoconf --force
configure.ac:3782: warning: AC_LIBTOOL_PROG_COMPILER_PIC is m4_require'd but not m4_defun'd
acinclude.m4:2363: GMP_ASM_X86_GOT_UNDERSCORE is expanded from...
configure.ac:3782: the top level
configure.ac:3785: warning: AC_ENABLE_SHARED is m4_require'd but not m4_defun'd
acinclude.m4:2717: GMP_ASM_X86_MCOUNT is expanded from...
configure.ac:3785: the top level
configure.ac:3785: warning: AC_PROG_LIBTOOL is m4_require'd but not m4_defun'd
acinclude.m4:2717: GMP_ASM_X86_MCOUNT is expanded from...
configure.ac:3785: the top level
configure.ac:2663: error: possibly undefined macro: AC_LIBTOOL_WIN32_DLL
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
configure.ac:2753: error: possibly undefined macro: AC_PROG_LIBTOOL
configure.ac:2876: error: possibly undefined macro: AC_CHECK_LIBM
configure:11252: error: possibly undefined macro: AC_PROG_NM
configure:15215: error: possibly undefined macro: AC_LIBTOOL_PROG_COMPILER_PIC
configure:15310: error: possibly undefined macro: AC_ENABLE_SHARED
autoreconf: /usr/bin/autoconf failed with exit status: 1

But looks fine on Windows, I forgot to add the include dir for nettle, which is solved now.
Didn't test MacOS.

@Nemirtingas
Copy link
Contributor Author

Thanks for your quick fix.
LGTM now.
Could you please help confirm if you have tested the feature nettle? Does it work fine? If you didn't test it, I will do this later.

Nettle has issue when building on Linux.

Nettle works on Linux too, it appears that you need the native autoconf, libtool and gettext installed. (apt install autoconf libtool gettext).

@Nemirtingas
Copy link
Contributor Author

One last little fix. The nettle target Nettle::Nettle didn't exist. Instead it is just called nettle.

@Nemirtingas
Copy link
Contributor Author

Looks like the macos runner has crashed.

@Nemirtingas
Copy link
Contributor Author

Hi,
Any news about this ?

@NancyLi1013
Copy link
Contributor

Hi @Nemirtingas

Thanks for your kindly reminder. Sorry for the delay.
I will merge master to this PR to retrigger CI.

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Oct 22, 2020
@NancyLi1013
Copy link
Contributor

LGTM now. Thanks for this PR.

@Nemirtingas
Copy link
Contributor Author

Thanks.

@ras0219-msft ras0219-msft merged commit 5b93dab into microsoft:master Oct 23, 2020
@ras0219-msft
Copy link
Contributor

LGTM, thanks for the PR and fixing all the comments :)

@Nemirtingas Nemirtingas deleted the libjuice_port branch October 23, 2020 10:18
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.

5 participants