all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Pierre Neidhardt <mail@ambrevar.xyz>
To: Mike Rosset <mike.rosset@gmail.com>
Cc: 35866@debbugs.gnu.org
Subject: [bug#35866] [PATCH] gnu: Add qtwebengine.
Date: Thu, 19 Dec 2019 19:29:40 +0100	[thread overview]
Message-ID: <877e2sf9t7.fsf@ambrevar.xyz> (raw)
In-Reply-To: <20191218203223.22861-1-mike.rosset@gmail.com> (Mike Rosset's message of "Wed, 18 Dec 2019 12:32:23 -0800")

[-- Attachment #1: Type: text/plain, Size: 5890 bytes --]

Looking good!

Comments below:

> qtwebengine now inherits qtsvg
>
> This also uses substitute-keyword-arguments to only replace configure phase.

I think those 2 lines can be omitted or maybe rephrased.  We didn't have
qtwebengine before, so you'd need to explain what's up with qtsvg, why
it was problematic to use it.

> +(define-public qtwebengine
> +  (package (inherit qtsvg)
> +    (name "qtwebengine")
> +    (version (package-version qtbase))
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri
> +        (string-append "https://download.qt.io/official_releases/qt/"
> +                       (substring version 0 4)

Use (version-major+minor ...) or similar.

> +                       "/" version "/submodules/"
> +                       (string-append name "-everywhere-src-" version)
> +                       ".tar.xz"))
> +       (sha256
> +        (base32
> +         "08c60nh95m98mcqk444axs76xi6m9x0wvdxrzk9c2cxwqdbz59fa"))))
> +    (build-system gnu-build-system)
> +    (native-inputs
> +     `(("bison" ,bison)
> +       ("flex" ,flex)
> +       ("gperf" ,gperf)
> +       ("ninja" ,ninja)
> +       ("perl" ,perl)
> +       ("pkg-config" ,pkg-config)
> +       ("python-2" ,python-2)
> +       ("ruby" ,ruby)
> +       ))

Dangling parentheses.

> +    (inputs
> +     `(("alsa-lib" ,alsa-lib)
> +       ("atk" ,atk)
> +       ("cups-minimal" ,cups-minimal)
> +       ("dbus" ,dbus)
> +       ("ffmpeg" ,ffmpeg)
> +       ("fontconfig" ,fontconfig)
> +       ("harbuzz" ,harfbuzz)
> +       ("icu4c" ,icu4c)
> +       ("jsoncpp" ,jsoncpp)
> +       ("lcms" ,lcms)
> +       ("libcap" ,libcap)
> +       ("libevent" ,libevent)
> +       ("libgcrypt" ,libgcrypt)
> +       ("libjpeg" ,libjpeg-turbo)
> +       ;; libsrtp not found by qmake. seems to not have config.tests?
> +       ("libsrtp" ,libsrtp)

What does it entail?  Is libsrtp ignored then?
If there is a problem, please add a "FIXME:" comment or similar.

> +       ;; FIXME: error: ?struct vpx_svc_ref_frame_config? has no member named ?frame_flags?
> +       ;; ("libvpx" ,libvpx)
> +       ("libwebp" ,libwebp)
> +       ("libx11" ,libx11)
> +       ("libxcb" ,libxcb)
> +       ("libxcomposite" ,libxcomposite)
> +       ("libxcursor" ,libxcursor)
> +       ("libxi" ,libxi)
> +       ("libxkbcommon" ,libxkbcommon)
> +       ;; libxml not found due to icu not enabled in libxml?

You mean libxml2?
Is it ignored then?
Same as above, prefix with a FIXME if needed.

> +       ("libxml2" ,libxml2)
> +       ("libxrandr" ,libxrandr)
> +       ("libxrender" ,libxrender)
> +       ("libxslt" ,libxslt)
> +       ("libxtst" ,libxtst)
> +       ("mesa" ,mesa)
> +       ("minizip" ,minizip)
> +       ("nss" ,nss)
> +       ("opus" ,opus)
> +       ("pciutils" ,pciutils)
> +       ("protobuf" ,protobuf)
> +       ;; ("pulseaudio" ,pulseaudio)

Please explain why you left pulseaudio commented out.

> +       ("qtbase" ,qtbase)
> +       ("qtdeclarative" ,qtdeclarative)
> +       ("qtmultimedia" ,qtmultimedia)
> +       ("qtwebchannel" ,qtwebchannel)
> +       ("re2" ,re2)
> +       ("snappy" ,snappy)
> +       ("udev" ,eudev)
> +       ("xcb-util" ,xcb-util)))
> +    (arguments
> +     (substitute-keyword-arguments (package-arguments qtsvg)
> +       ((#:phases phases)
> +        `(modify-phases ,phases
> +           (replace 'configure
> +             (lambda* (#:key inputs outputs #:allow-other-keys)
> +               ;; Avoids potential race conditions
> +               (setenv "PYTHONDONTWRITEBYTECODE" "1")
> +               ;; Make build output less verbose
> +               (setenv "NINJAFLAGS" "-k1")
> +               ;; Use this output translations not qtbase

This comment is not very clear, maybe it could be rephrased, e.g.

"Replace the qtbase translations with those from qtwebengine.

> +               (substitute* "src/core/web_engine_library_info.cpp"
> +                 (("QLibraryInfo::location\\(QLibraryInfo::TranslationsPath\\)")
> +                  (format #f "QLatin1String(\"~a\")" (string-append (assoc-ref outputs "out") "/share/qt5/translations")))
> +                 (("QLibraryInfo::location\\(QLibraryInfo::DataPath\\)")
> +                  (format #f "QLatin1String(\"~a\")" (string-append (assoc-ref outputs "out") "/share/qt5"))))
> +               ;; Substitute full dynamic path for nss
> +               (substitute* "src/3rdparty/chromium/crypto/nss_util.cc"
> +                 (("libnssckbi.so")
> +                  (string-append (assoc-ref inputs "nss") "/lib/nss/libnssckbi.so")))
> +               ;; Substitute full dynamic path for udev
> +               (substitute* "src/3rdparty/chromium/device/udev_linux/udev1_loader.cc"
> +                 (("libudev.so.1")
> +                  (string-append (assoc-ref inputs "udev") "/lib/libudev.so.1")))
> +               ;; Valid QT_BUILD_PARTS variables are:
> +               ;; libs tools tests examples demos docs translations
> +               (invoke "qmake" "QT_BUILD_PARTS = libs tools" "--"
> +                       "--webengine-printing-and-pdf=no"
> +                       "--webengine-ffmpeg=system"
> +                       "--webengine-icu=system"
> +                       "--webengine-pepper-plugins=no")))))
> +       ((#:tests? _ #f) #f)))

No tests?

> +    (native-search-paths
> +     (list (search-path-specification
> +            (file-type 'regular)
> +            (separator #f)
> +            (variable "QTWEBENGINEPROCESS_PATH")
> +            (files '("lib/qt5/libexec/QtWebEngineProcess")))))
> +    (home-page "https://www.qt.io")
> +    (synopsis "Qt WebEngine Module")

Probably no capital for "module."

Great work, thanks again!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

  reply	other threads:[~2019-12-19 18:30 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-23  6:23 [bug#35866] [PATCH] gnu: Add qtwebengine mike rosset
2019-05-27 21:05 ` Ludovic Courtès
2019-05-28 17:48   ` mrosset
2019-06-01  9:46     ` Ludovic Courtès
2019-05-29 23:37 ` [bug#35866] Mike Rosset
2019-05-29 23:37   ` [bug#35866] [PATCH] gnu: qtwebengine: Do not use 3rd party libraries Mike Rosset
2019-05-31 19:28     ` Marius Bakke
2019-05-31 23:16       ` mrosset
2019-07-18  9:26 ` [bug#35866] [PATCH] gnu: Add qtwebengine Ricardo Wurmus
2019-10-06 15:45   ` Pierre Neidhardt
2019-10-07 15:49     ` mike.rosset
2019-07-24 14:06 ` Brendan Tildesley
2019-12-16 15:29 ` [bug#35866] " Hartmut Goebel
2019-12-16 16:02   ` mike.rosset
2019-12-16 18:01     ` Pierre Neidhardt
2019-12-18 20:32 ` [bug#35866] [PATCH] " Mike Rosset
2019-12-19 18:29   ` Pierre Neidhardt [this message]
2019-12-19 20:19   ` Ricardo Wurmus
2019-12-20  0:02     ` mike.rosset
2019-12-18 20:45 ` Mike Rosset
2019-12-31 13:10   ` Hartmut Goebel
2020-01-01 16:29     ` mike.rosset
2020-01-01 16:03 ` [bug#35866] [PATCH 1/4] " Mike Rosset
2020-01-01 16:03   ` [bug#35866] [PATCH 2/4] gnu: qtwebengine now uses pulseaudio Mike Rosset
2020-01-01 16:03   ` [bug#35866] [PATCH 3/4] gnu: qtwebengine: now uses system libvpx Mike Rosset
2020-01-01 16:03   ` [bug#35866] [PATCH 4/4] gnu: qtwebengine: separate substitution and configure phase Mike Rosset
2020-01-24  0:16 ` bug#35866: Status: [PATCH] gnu: Add qtwebengine Mike Rosset

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877e2sf9t7.fsf@ambrevar.xyz \
    --to=mail@ambrevar.xyz \
    --cc=35866@debbugs.gnu.org \
    --cc=mike.rosset@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.