Bug 2231252 - Review Request: yass - lightweight and secure http/socks proxy
Summary: Review Request: yass - lightweight and secure http/socks proxy
Keywords:
Status: ASSIGNED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Benson Muite
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/Chilledheart/yass
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-08-11 06:18 UTC by Keyue Hu
Modified: 2023-08-17 06:15 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:
benson_muite: fedora-review?


Attachments (Terms of Use)
The .spec file difference from Copr build 6265737 to 6265743 (392 bytes, patch)
2023-08-11 08:21 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 6265743 to 6271270 (3.77 KB, patch)
2023-08-14 02:56 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 6271272 to 6271304 (295 bytes, patch)
2023-08-14 03:39 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 6271304 to 6310972 (1.92 KB, patch)
2023-08-17 06:15 UTC, Fedora Review Service
no flags Details | Diff

Description Keyue Hu 2023-08-11 06:18:50 UTC
Spec URL: https://github.com/Chilledheart/yass/releases/download/1.3.14/yass.spec
SRPM URL: https://github.com/Chilledheart/yass/releases/download/1.3.14/yass-1.3.14-0.fc37.src.rpm
Description: yass is initiated as C++ rewrite of the outdated shadowsocks-libev package and provide the similar functionalities. During the recent development, it also supports naiveproxy protocol which is more efficient protocol. Compared with
shadowsocks-libev, it not only contains the client cli command and server cli
command, but also it contains a gtk3/gtk4 (both are supported) graphical
interface which is more friendly to the new users.
Fedora Account System Username:

Comment 1 Fedora Review Service 2023-08-11 06:29:44 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6265622
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06265622-yass/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 2 Keyue Hu 2023-08-11 06:38:08 UTC
build failed because the unittests require internet access (DNS related), 
it can be easily by-passed with ./yass_test --no_cares_tests

Comment 4 Fedora Review Service 2023-08-11 06:52:54 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6265633
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06265633-yass/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 6 Fedora Review Service 2023-08-11 07:09:37 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6265645
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06265645-yass/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 7 Keyue Hu 2023-08-11 07:20:58 UTC
Try new spec again.

Spec URL: https://github.com/Chilledheart/yass/releases/download/1.3.14/yass-nocares.spec

Comment 9 Fedora Review Service 2023-08-11 07:43:23 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6265708
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06265708-yass/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 12 Fedora Review Service 2023-08-11 07:58:46 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6265737
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06265737-yass/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 13 Fedora Review Service 2023-08-11 08:21:33 UTC
Created attachment 1982956 [details]
The .spec file difference from Copr build 6265737 to 6265743

Comment 14 Fedora Review Service 2023-08-11 08:21:35 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6265743
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06265743-yass/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 15 Benson Muite 2023-08-11 13:28:00 UTC
Thanks for adding this to Fedora. Initial comments:
a) Can you use SPDX identifier
GPL-2.0-only
or
GPL-2.0-or-later
See
https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_valid_license_short_names

b) Perhaps add
BuildRequires: aspio-devel

c) Some third party code is distributed with the tarball and built:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06265743-yass/builder-live.log.gz
Perhaps indicate what is bundled and explain why a version packaged in Fedora cannot be used, see:
https://packages.fedoraproject.org/pkgs/gtest/gtest
https://packages.fedoraproject.org/pkgs/http-parser/http-parser
https://packages.fedoraproject.org/pkgs/abseil-cpp/abseil-cpp
https://packages.fedoraproject.org/pkgs/google-benchmark/google-benchmark
https://packages.fedoraproject.org/pkgs/xxhash/xxhash

BoringSSL is not packaged, but maybe OpenSSL can be used instead:
https://packages.fedoraproject.org/pkgs/openssl/openssl

Quiche is not available:
https://github.com/google/quiche
but probably worth packaging separately. Though it currently uses Bazel which is only available as a copr:
https://copr.fedorainfracloud.org/coprs/vbatts/bazel/
The build seems to use CMake, so maybe something that can be contributed upstream.

Other implementations listed at:
https://github.com/quicwg/base-drafts/wiki/Implementations

Possibly easier to package are:
https://github.com/h2o/quicly
https://github.com/p-quic/pquic
https://github.com/private-octopus/picoquic
https://github.com/alibaba/xquic (Depends on BabaSSL/Tongsou or BoringSSL)

Maybe also useful might be:
https://packages.fedoraproject.org/search?query=quic

d) Can Fedora build macros be used:
%cmake
%cmake_build
%cmake_install

See
https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/

e) Is golang needed? It seems Go code is used only in tools/ and this has dependencies which are not included.

f) The line
Source0: https://github.com/Chilledheart/yass/archive/refs/tags/%{version}.tar.gz
should be
Source0: %{url}/archive/%{version}/%{name}-%{version}.tar.gz

see
https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags
Though the srpm seems to use full git source rather than a commit as third party code is included.

Comment 16 Keyue Hu 2023-08-11 17:32:31 UTC
(In reply to Benson Muite from comment #15)
> Thanks for adding this to Fedora. Initial comments:

Thanks for your work.


> a) Can you use SPDX identifier
> GPL-2.0-only
> or
> GPL-2.0-or-later
> See
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> LicensingGuidelines/#_valid_license_short_names

It’s GPL-2.0-only. There is a COPYING file under the top directory of source documenting it.

> 
> b) Perhaps add
> BuildRequires: aspio-devel

I don’t know what’s aspio doing but if you mean asio, it is almost header-only library. (Does it get packaged in fedora as well?)

> 
> c) Some third party code is distributed with the tarball and built:
> https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-
> review-2231252-yass/fedora-rawhide-x86_64/06265743-yass/builder-live.log.gz
> Perhaps indicate what is bundled and explain why a version packaged in
> Fedora cannot be used, see:
> https://packages.fedoraproject.org/pkgs/gtest/gtest
> https://packages.fedoraproject.org/pkgs/http-parser/http-parser
> https://packages.fedoraproject.org/pkgs/abseil-cpp/abseil-cpp
> https://packages.fedoraproject.org/pkgs/google-benchmark/google-benchmark
> https://packages.fedoraproject.org/pkgs/xxhash/xxhash

Thanks for pointing it out. There are two major obstacles from adopting system libraries, one is that it is a cross-platform project supporting msvc(visual c++), mingw32, Linux, musl-based Linux, FreeBSD and macOS targets. There are some reasons to keep things in one place and make it work across different platforms. The other is that yass largely depend on c++ libraries where some of them has no stable ABI which means things break on upgrading or downgrading the shared library.

googletest and google-benchmark are on test purposes and c++ based. The api might vary between even two snapshots, so it is built from source if possible.

For abseil-cpp, it is also c++ based. We are using the latest lts branch and from previous experience, we really don’t want to move from it as it might break sanitizers tests such as memory sanitizer. See https://clang.llvm.org/docs/MemorySanitizer.html.

As to xxhash, we just bumped to lastest release 0.8.2. If fedora switches to it already, we can move to the system one. Otherwise, something might break as int64x2 or some definition is not found in some platforms.

For http-parser, I really can’t catch up whether it is the same thing in yass project. The http-parser used comes from node project and not maintained for some years (get replaced by another parser in typed script). We can catch it up if it won’t get rid from fedora.

> 
> BoringSSL is not packaged, but maybe OpenSSL can be used instead:
> https://packages.fedoraproject.org/pkgs/openssl/openssl

Quiche library is bound to boringssl and we don’t have alternatives.

> 
> Quiche is not available:
> https://github.com/google/quiche
> but probably worth packaging separately. Though it currently uses Bazel
> which is only available as a copr:
> https://copr.fedorainfracloud.org/coprs/vbatts/bazel/
> The build seems to use CMake, so maybe something that can be contributed
> upstream.

Quiche is out from google internal library and used for two open source projects namely, chromium and envoy. You can start from it if want to make it a distributable package.

I don’t believe there is a stable ABI because they breaks things from time to time. 

Officially, they support gn and bazel build system but only part of it is open source. If you are looking at bazel, you can’t miss envoy’s code. They separate every single library there.

If I miss something here, please point it out in reply. Thanks in advance.


> 
> Other implementations listed at:
> https://github.com/quicwg/base-drafts/wiki/Implementations
> 
> Possibly easier to package are:
> https://github.com/h2o/quicly
> https://github.com/p-quic/pquic
> https://github.com/private-octopus/picoquic
> https://github.com/alibaba/xquic (Depends on BabaSSL/Tongsou or BoringSSL)

I must admit I don’t be familiar with most of them. But there are many quiche libraries such the one from cloudflare is in golang and not related.

https://github.com/cloudflare/quiche

> 
> Maybe also useful might be:
> https://packages.fedoraproject.org/search?query=quic
> 
> d) Can Fedora build macros be used:
> %cmake
> %cmake_build
> %cmake_install
> 
> See
> https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/

We are passing many custom options to cmake. If the rpm macros can work instead, we should replace them.

> 
> e) Is golang needed? It seems Go code is used only in tools/ and this has
> dependencies which are not included.

yes, it is mimic required but besides the tool directory, it is used in boringssl to generate error-handling c code.

> 
> f) The line
> Source0:
> https://github.com/Chilledheart/yass/archive/refs/tags/%{version}.tar.gz
> should be
> Source0: %{url}/archive/%{version}/%{name}-%{version}.tar.gz
> 
> see
> https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
> #_git_tags
> Though the srpm seems to use full git source rather than a commit as third
> party code is included.

No worry, the upstream will upload the git whole archive without history in every release such as 1.3.14 release, there is 

https://github.com/Chilledheart/yass/releases/download/1.3.14/yass-1.3.14.tar.gz

I will correct the url in next update.

Comment 17 Keyue Hu 2023-08-12 04:51:22 UTC
envoy project: https://www.envoyproxy.io/
envoy's bazel file for quiche: https://github.com/envoyproxy/envoy/blob/main/bazel/external/quiche.bzl

Comment 18 Keyue Hu 2023-08-14 02:52:09 UTC
Changelog: update license field in spec and xxhash, http-parser dependents.

Spec URL: https://github.com/Chilledheart/copr-yass/releases/download/1.3.14-34/yass.spec
SRPM URL: https://github.com/Chilledheart/copr-yass/releases/download/1.3.14-34/yass-1.3.14-34.fc37.src.rpm

Comment 19 Fedora Review Service 2023-08-14 02:56:03 UTC
Created attachment 1983230 [details]
The .spec file difference from Copr build 6265743 to 6271270

Comment 20 Fedora Review Service 2023-08-14 02:56:05 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6271270
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06271270-yass/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 22 Fedora Review Service 2023-08-14 03:04:35 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6271272
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06271272-yass/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 24 Fedora Review Service 2023-08-14 03:39:55 UTC
Created attachment 1983232 [details]
The .spec file difference from Copr build 6271272 to 6271304

Comment 25 Fedora Review Service 2023-08-14 03:39:56 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6271304
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06271304-yass/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 26 Keyue Hu 2023-08-14 08:11:09 UTC
There is also a third party library json-devel (aka, https://github.com/nlohmann/json https://fedora.pkgs.org/37/fedora-x86_64/json-devel-3.11.2-1.fc37.x86_64.rpm.html) bundled.
However, it is header-only library I don't think it is forced to use the system one, right?

Comment 27 Benson Muite 2023-08-15 08:35:54 UTC
System libraries are much preferred, even if header only.  Expect one of the
reasons for this is that if there is an issue with the library, then one can
much more easily find all the packages that depend on it and have built using
it and then update them.

Automated review at:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06271304-yass/fedora-review/review.txt
gives the following warning:
yass.x86_64: W: file-not-in-%lang /usr/share/locale/en/LC_MESSAGES/yass.mo

Comment 28 Keyue Hu 2023-08-17 05:46:04 UTC
(In reply to Benson Muite from comment #27)
> System libraries are much preferred, even if header only.  Expect one of the
> reasons for this is that if there is an issue with the library, then one can
> much more easily find all the packages that depend on it and have built using
> it and then update them.

Okay, I'll update the spec to use header-only nlohmann/json (i.e. json-devel in fedora repository) library.
For other c++ libraries(such as boringssl, quiche, abseil-cpp, googletest and googlemock), I think it is better to use bundled one.

> 
> Automated review at:
> https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-
> review-2231252-yass/fedora-rawhide-x86_64/06271304-yass/fedora-review/review.
> txt
> gives the following warning:
> yass.x86_64: W: file-not-in-%lang /usr/share/locale/en/LC_MESSAGES/yass.mo

It seems good that %lang works well even in RHEL7/CentOS7.

Changelog: Fixed %lang usage and json-devel along with other upstream changes.

Spec URL: https://github.com/Chilledheart/copr-yass/releases/download/1.3.14-52/yass.spec
SRPM URL: https://github.com/Chilledheart/copr-yass/releases/download/1.3.14-52/yass-1.3.14-52.fc37.src.rpm

Comment 29 Fedora Review Service 2023-08-17 06:15:16 UTC
Created attachment 1983734 [details]
The .spec file difference from Copr build 6271304 to 6310972

Comment 30 Fedora Review Service 2023-08-17 06:15:19 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6310972
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231252-yass/fedora-rawhide-x86_64/06310972-yass/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.


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