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

halide: 10.0.0 -> 14.0.0 #180015

Merged
merged 2 commits into from
Aug 25, 2022
Merged

halide: 10.0.0 -> 14.0.0 #180015

merged 2 commits into from
Aug 25, 2022

Conversation

AtilaSaraiva
Copy link
Contributor

@AtilaSaraiva AtilaSaraiva commented Jul 3, 2022

Description of changes

Major changes 13.0.3 -> 14.0.0
@abadams
Add ability to pass a user context in JIT mode (halide/Halide#6313)
Reenable warning about unscheduled update definitions (halide/Halide#6602)
@alexreinking
Add helper for cross-compiling Halide generators. (halide/Halide#6366)
@LebedevRI
Implement SanitizerCoverage support (Refs. halide/Halide#6513) (halide/Halide#6517)
@steven-johnson
Expand optional static-typing for Buffer to include dimensionality (halide/Halide#6574)
Deprecate the Generator::build() method (halide/Halide#6580)
Move GeneratorContext into a standalone class (halide/Halide#6618)
Python Bindings didn't allow for zero-D Funcs, ImageParams, Buffers (halide/Halide#6633)
@zvookin
Timer based profiler (halide/Halide#6642)

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Result of nixpkgs-review run on x86_64-linux 1

1 package failed to build:
  • hdr-plus
1 package built:
  • halide

@LebedevRI
Copy link

FTR, while i don't use NixOS, it would be nice if to have this package properly kept up-to-date.

@AtilaSaraiva
Copy link
Contributor Author

Don't worry because it will haha @LebedevRI

Copy link
Contributor

@ck3d ck3d 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 to me

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

LGTM

@AndersonTorres
Copy link
Member

OfBorg failing on Darwin.

@AtilaSaraiva
Copy link
Contributor Author

I know it is compatible with darwin but I have no idea of how to make it work :/. maybe we should limit it to linux? dunno.

'';

cmakeFlags = [ "-DWARNINGS_AS_ERRORS=OFF" "-DWITH_PYTHON_BINDINGS=OFF" ];
cmakeFlags = [ "-DWARNINGS_AS_ERRORS=OFF" "-DWITH_PYTHON_BINDINGS=OFF" "-DTARGET_WEBASSEMBLY=OFF" ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmakeFlags = [ "-DWARNINGS_AS_ERRORS=OFF" "-DWITH_PYTHON_BINDINGS=OFF" "-DTARGET_WEBASSEMBLY=OFF" ];
cmakeFlags = [
"-DWARNINGS_AS_ERRORS=OFF"
"-DWITH_PYTHON_BINDINGS=OFF"
"-DTARGET_WEBASSEMBLY=OFF"
];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx for the review, fixed!


cmakeFlags = [ "-DWARNINGS_AS_ERRORS=OFF" "-DWITH_PYTHON_BINDINGS=OFF" ];
cmakeFlags = [
"-DWARNINGS_AS_ERRORS=OFF"

Choose a reason for hiding this comment

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

This setting no longer exists in the Halide build

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"-DWARNINGS_AS_ERRORS=OFF"

@AtilaSaraiva
Copy link
Contributor Author

I think this is ready for merge folks.

buildInputs = [
llvmPackages.llvm
llvmPackages.lld
llvmPackages.openmp
llvmPackages.libclang
libpng
libjpeg
mesa
Copy link
Member

Choose a reason for hiding this comment

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

You are sure mesa does not go into nativeBuildInputs?

Choose a reason for hiding this comment

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

I'm not sure why mesa is here at all? It's not a requirement to build Halide. It's probably a requirement to build apps using Halide's OpenGL Compute backend, though.

Copy link
Member

@SuperSandro2000 SuperSandro2000 Jul 7, 2022

Choose a reason for hiding this comment

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

hmmm... Maybe this should be part of a wrapper then?

buildInputs are not propagated so if it is unused in the build process it should do nothing.

Copy link
Contributor Author

@AtilaSaraiva AtilaSaraiva Jul 7, 2022

Choose a reason for hiding this comment

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

It would be interesting to have a wrapper for halide dependent projects, but we have to think about how to best design this wrapper to fit the needs of a specific app first.

for now I will remove the mesa dependency.

@AtilaSaraiva
Copy link
Contributor Author

Done!

@risicle
Copy link
Contributor

risicle commented Jul 10, 2022

Fails to build for me on macos 10.15:

Undefined symbols for architecture x86_64:
  "operator delete(void*)", referenced from:
      dump_header(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in build_halide_h.cpp.o
      _main in build_halide_h.cpp.o
      std::__1::__tree<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >::destroy(std::__1::__tree_node<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, void*>*) in build_halide_h.cpp.o
      std::__1::pair<std::__1::__tree_iterator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__tree_node<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, void*>*, long>, bool> std::__1::__tree<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >::__emplace_unique_key_args<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in build_halide_h.cpp.o
  "operator new(unsigned long)", referenced from:
      dump_header(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in build_halide_h.cpp.o
      _main in build_halide_h.cpp.o
      std::__1::pair<std::__1::__tree_iterator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__tree_node<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, void*>*, long>, bool> std::__1::__tree<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >::__emplace_unique_key_args<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in build_halide_h.cpp.o
  "___cxa_allocate_exception", referenced from:
      std::__1::__throw_length_error(char const*) in build_halide_h.cpp.o
  "___cxa_free_exception", referenced from:
      std::__1::__throw_length_error(char const*) in build_halide_h.cpp.o
  "___cxa_throw", referenced from:
      std::__1::__throw_length_error(char const*) in build_halide_h.cpp.o
  "___gxx_personality_v0", referenced from:
      dump_header(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in build_halide_h.cpp.o
      _main in build_halide_h.cpp.o
      std::__1::pair<std::__1::__tree_iterator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__tree_node<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, void*>*, long>, bool> std::__1::__tree<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >::__emplace_unique_key_args<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in build_halide_h.cpp.o
      std::__1::__throw_length_error(char const*) in build_halide_h.cpp.o
      Dwarf Exception Unwind Info (__eh_frame) in build_halide_h.cpp.o
ld: symbol(s) not found for architecture x86_64

@AndersonTorres
Copy link
Member

Maybe in MacOS it should be the case to force GCC compiler instead of default Clang?

@alexreinking
Copy link

Maybe in MacOS it should be the case to force GCC compiler instead of default Clang?

Halide is absolutely expected to buildable with Clang (and GCC). We should get to the bottom of this. I haven't seen this particular linker error while building Halide before.

@ck3d
Copy link
Contributor

ck3d commented Jul 14, 2022

The darwin issue is related to #166205 . We should merge this PR and hope the problem is fixed in llvm.

@risicle
Copy link
Contributor

risicle commented Jul 14, 2022

That would leave macos halide users with a broken package for an unknown period of time.

@AtilaSaraiva
Copy link
Contributor Author

The current one is broken for both mac and linux afaik, so why not at least make it workable for linux?

@SuperSandro2000
Copy link
Member

Under the assumption that it worked before we shouldn't hard break it for a platform but if it didn't work then it isn't a problem if it is still not working.

@risicle
Copy link
Contributor

risicle commented Jul 15, 2022

https://hydra.nixos.org/eval/1771618?filter=halide&compare=1771575&full=#tabs-still-succeed suggests it's only broken for x86_64 linux, which is an unusual situation TBH.

@AtilaSaraiva
Copy link
Contributor Author

Let's see if something changed.

@SuperSandro2000 SuperSandro2000 merged commit f3228e7 into NixOS:master Aug 25, 2022
@AtilaSaraiva AtilaSaraiva deleted the halide branch August 25, 2022 19:54
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.

8 participants