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

Update to 71.0.3578.80 #618

Merged
merged 15 commits into from Dec 9, 2018
Merged

Update to 71.0.3578.80 #618

merged 15 commits into from Dec 9, 2018

Conversation

xsmile
Copy link
Contributor

@xsmile xsmile commented Dec 7, 2018

  • removed gn-bootstrap-remove-gn-gen.patch, --skip_generate_buildfiles can now be used to skip this step
  • removed add-flag-to-show-avatar-button.patch, the new UI places the avatar button in line with the settings button instead of the title bar and switching to the old UI is not possible anymore

@ghost
Copy link

ghost commented Dec 7, 2018

Thank you. It builds fine for me.

@qvint
Copy link
Contributor

qvint commented Dec 8, 2018

  • removed add-flag-to-show-avatar-button.patch, the new UI places the avatar button in line with the settings button instead of the title bar and switching to the old UI is not possible anymore

Does the patch stop working in version 71? Did you check that? It works for both UIs in version 70.

@perfect7gentleman
Copy link

 ~ $ chromium

(chromium-browser:17169): Gtk-WARNING **: 18:08:42.659: Theme parsing error: gtk.css:68:35: The style property GtkButton:child-displacement-x is deprecated and shouldn't be used anymore. It will be removed in a future version

(chromium-browser:17169): Gtk-WARNING **: 18:08:42.659: Theme parsing error: gtk.css:69:35: The style property GtkButton:child-displacement-y is deprecated and shouldn't be used anymore. It will be removed in a future version

(chromium-browser:17169): Gtk-WARNING **: 18:08:42.659: Theme parsing error: gtk.css:73:46: The style property GtkScrolledWindow:scrollbars-within-bevel is deprecated and shouldn't be used anymore. It will be removed in a future version
Aborted

@ghost
Copy link

ghost commented Dec 8, 2018

@perfect7gentleman Those warnings comes from the theme engine, I think, and shouldn't cause an abort. On another issue tracker, I saw that you are using Clang as the default toolchain, yes? If so, you should double-check if you properly built chromium's dependencies, specially C++ ones.

I had similar issue when tried to build against libcxx/libcxxabi.

@perfect7gentleman
Copy link

perfect7gentleman commented Dec 8, 2018

@ian-moone , Clang is default for chromium deps such as nodejs and others. New dep introduced in 71, jsoncpp, is also built with Clang.

 ~ $ cat /etc/portage/env/O3_clang 
CC="clang"
CXX="clang++"
CFLAGS="-march=native -mtune=native -O3 -pipe -fomit-frame-pointer -fno-stack-protector -flto=thin -flto-jobs=9 -fuse-ld=lld"
CXXFLAGS="${CFLAGS} -stdlib=libc++"
LDFLAGS="-Wl,-O2 -Wl,--as-needed -Wl,--strip-debug -Wl,--thinlto-jobs=9 -flto=thin -fuse-ld=lld"
AR="llvm-ar"
NM="llvm-nm"
RANLIB="llvm-ranlib"
 ~ $ emerge -avtp clang llvm libcxx libcxxabi

[ebuild   R   #] sys-devel/clang-7.0.1_rc2:7::gentoo  USE="default-compiler-rt default-libcxx -debug -doc -static-analyzer -test -xml (-z3)" LLVM_TARGETS="(X86) -AArch64 -AMDGPU -ARM -BPF -Hexagon -Lanai -MSP430 -Mips -NVPTX -PowerPC -Sparc -SystemZ -XCore" PYTHON_TARGETS="python2_7" 13,076 KiB
[ebuild   R   #]  sys-libs/libcxx-7.0.1_rc2::gentoo  USE="libcxxabi libunwind -libcxxrt -static-libs -test" 1,603 KiB
[ebuild   R   #]   sys-libs/libcxxabi-7.0.1_rc2::gentoo  USE="libunwind -static-libs -test" 523 KiB
[ebuild   R   #]    sys-devel/llvm-7.0.1_rc2:7::gentoo  USE="gold libffi ncurses -debug -doc -exegesis -libedit -test -xar -xml" LLVM_TARGETS="(X86) -AArch64 -AMDGPU -ARM -BPF -Hexagon -Lanai -MSP430 -Mips -NVPTX -PowerPC -Sparc -SystemZ -XCore" 27,642 KiB

@xsmile
Copy link
Contributor Author

xsmile commented Dec 8, 2018

Does the patch stop working in version 71? Did you check that? It works for both UIs in version 70.

I checked it briefly and it requires updating as code referencing the old UI was removed. If needed, I can update and include it again.

@ghost
Copy link

ghost commented Dec 8, 2018

Have you successfully built chromium against libcxx/libxxabi in the past? Because I don't. I had to build dev-libs/re2, dev-libs/jsoncpp and of course, chromium itself with:

CXXFLAGS="${CFLAGS} -stdlib=libstdc++"
LDFLAGS="-Wl,-O2 -Wl,--as-needed -fuse-ld=lld -Wl,-lgcc_s"

@perfect7gentleman
Copy link

perfect7gentleman commented Dec 8, 2018

ave you successfully built chromium against libcxx/libxxabi in the past?

yes, and lastest ungoogled-chromium-70 too.

I had to build dev-libs/re2, dev-libs/jsoncpp

they are built with Clang

@ghost
Copy link

ghost commented Dec 8, 2018

I'm not sure if chromium truly supports system libcxx on linux yet. At least looking at the build/config/compiler/BUILD.gn, libc++ are only related to Mac, like this one:

  if (is_mac) {
    # The system libc++ on Mac doesn't have aligned allocation in C++17.
    defines += [ "_LIBCPP_HAS_NO_ALIGNED_ALLOCATION" ]
    cflags_cc += [ "-stdlib=libc++" ]
    ldflags += [ "-stdlib=libc++" ]
  }

The other references are when use_custom_libcxx == true, which uses the bundled toolchain. So I suggest that you should stick to libstdc++ for now. Or at least try it, and see if that works for you.

@perfect7gentleman
Copy link

I don't see any big differences between ebuilds for 70 and 71. Only jsonccp as new dep.

@ghost
Copy link

ghost commented Dec 8, 2018

Yeah, but chromium's internals could've changed a lot between them.

@qvint
Copy link
Contributor

qvint commented Dec 8, 2018

@xsmile Please update the patch, it is fixed now.

@xsmile
Copy link
Contributor Author

xsmile commented Dec 8, 2018

@xsmile Please update the patch, it is fixed now.

Thank you, I included it again.

@@ -0,0 +1,23 @@
description: gcc6 cannot automatically resolve these overloads, tell it what to do
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is necessary to have this patch considering that all of this code will be removed in disable-google-host-detection.patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

# was reverted since it
# - breaks linux_portable.
# - has no clear side-effects (the include only defines constants).
# - has nothing to do with fontconfig.
Copy link
Member

Choose a reason for hiding this comment

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

Please don't revert this.

FYI, I've been trying to develop a new workflow for updating Debian-related patches and packaging files. It should be able to catch cases like this. The instructions for this workflow are here: https://github.com/ungoogled-software/ungoogled-chromium-debian. That being said, I don't think it is necessary to re-work this PR using that workflow. If you decide to try it out for the next release, let me know how it goes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, fixed.
Thanks for the instructions, I'll take a look next time.

@@ -0,0 +1,1655 @@
description: restore support for building against gtk2
Copy link
Member

Choose a reason for hiding this comment

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

I don't have any configurations that use the GTK2 UI. I'm guessing it is not necessary to include this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is optional, I removed the patch.

return;

// Delete any remaining pending requests on this Channel ID.
- pending_transactions_ = {};
+ pending_transactions_ = base::queue<std::pair<std::vector<uint8_t>, DeviceCallback>>();
Copy link
Member

Choose a reason for hiding this comment

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

I manually added this line back in because I wasn't sure if removing this line would cause any issues or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@Eloston
Copy link
Member

Eloston commented Dec 8, 2018

Whoops, I only wanted 8c49d06 to be approved again, not the entire PR...

@Eloston Eloston added this to the 71.x.x.x milestone Dec 9, 2018
Copy link
Member

@Eloston Eloston left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks.

@Eloston Eloston merged commit 907d2a3 into ungoogled-software:master Dec 9, 2018
@xsmile xsmile deleted the 71 branch January 31, 2019 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants