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

prusa-slicer: 2.8.0 -> 2.8.1 #358215

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

prusa-slicer: 2.8.0 -> 2.8.1 #358215

wants to merge 5 commits into from

Conversation

cbat98
Copy link

@cbat98 cbat98 commented Nov 22, 2024

This PR brings PrusaSlicer in line with the latest public release 2.8.1. Several new features such as UI improvements and a new infill type were added in this version. A full changelog can be found here:

I've also provided a patch file to fix some of the build issues I experienced whilst upgrading this package, however I'm still new to NixOS so any feedback is very welcome.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@cbat98 cbat98 changed the title Prusa prusa-slicer: 2.8.0 -> 2.8.1 Nov 22, 2024
@cbat98
Copy link
Author

cbat98 commented Nov 23, 2024

Result of nixpkgs-review pr 358215 run on x86_64-linux 1

4 packages failed to build:
  • super-slicer
  • super-slicer-beta
  • super-slicer-beta.debug
  • super-slicer.debug
4 packages built:
  • prusa-slicer
  • prusa-slicer.debug
  • super-slicer-latest
  • super-slicer-latest.debug

@cbat98
Copy link
Author

cbat98 commented Nov 23, 2024

The 4 packages that fail to build with nixpkgs-review also seem to fail to build on master using nix-build -A super-slicer. Therefore I believe fixing this falls out of scope of this PR.

@cbat98
Copy link
Author

cbat98 commented Nov 23, 2024

Would I be able to get some advice on this PR please?
@thorstenweber83
@hesiod

This is my first nix PR, be gentle :)

@jmickelin
Copy link

jmickelin commented Nov 23, 2024

Hi, I made a PR upstream to fix all these build issues. I expect it to be merged with the next alpha of 2.9.0, but it might be the case that it's backported to 2.8.*. Note that it replaces the patches used in the current derivation for 2.8.0, and sidesteps some of the more hacky solutions used elsewhere. See the issue for extensive discussions on what the problems were and why this is the right approach.

In any case, here is my overlay that incorporates all the fixes.

final: prev:
(prev.prusa-slicer.override {
    opencascade-occt_7_6 = prev.opencascade-occt_7_6.overrideAttrs {
      version = "7.6.1";
      src = prev.fetchurl {
        name = "occt-V7_6_1.tar.gz";
        url = "https://git.dev.opencascade.org/gitweb/?p=occt.git;a=snapshot;h=V7_6_1;sf=tgz";
        hash = "sha256-PZVrLSbR7nWsArIeg47YtHMdnpFAHK5K80VbVrAA9W0=";
      };
    };
}).overrideAttrs (final: old: rec {
  version = "2.8.1";
  src = prev.fetchFromGitHub {
    owner = "prusa3d";
    repo = "PrusaSlicer";
    hash = "sha256-nMLZvvZLIOChCLn8A9sOph1lqWsHb00eTG8z98/l0C8=";
    rev = "version_${version}";
  };

  buildInputs = old.buildInputs ++ [ prev.webkitgtk_4_0 ];

  prePatch = old.prePatch + ''
    rm cmake/modules/FindEigen3.cmake
    rm cmake/modules/FindDBus.cmake
  '';
  
  patches = [
    (prev.writeText "linker-fixes.patch" ''
       --- a/src/libslic3r/CMakeLists.txt
       +++ b/src/libslic3r/CMakeLists.txt
       @@ -596,6 +596,7 @@ target_link_libraries(libslic3r PUBLIC
            libigl
            agg
            ankerl
       +    boost_headeronly
        )
        
        if (APPLE)
       --- a/CMakeLists.txt
       +++ b/CMakeLists.txt
       @@ -246,7 +246,7 @@ if (CMAKE_SYSTEM_NAME STREQUAL "Linux")
            set(THREADS_PREFER_PTHREAD_FLAG ON)
            find_package(Threads REQUIRED)
       
       -    find_package(DBus REQUIRED)
       +    find_package(DBus1 REQUIRED)
        endif()
         
        if (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_GNUXX)
       --- a/src/slic3r/CMakeLists.txt
       +++ b/src/slic3r/CMakeLists.txt
       @@ -383,6 +383,7 @@ set(SLIC3R_GUI_SOURCES
        )
            
        find_package(NanoSVG REQUIRED)
       +find_package(OpenSSL REQUIRED)
            
        if (APPLE)
            list(APPEND SLIC3R_GUI_SOURCES
       @@ -438,6 +439,9 @@ target_link_libraries(
            NanoSVG::nanosvgrast
            stb_dxt
            fastfloat
       +    OpenSSL::SSL
       +    OpenSSL::Crypto
       +    boost_headeronly
        )
              
        if (MSVC))
       @@ -443,7 +443,7 @@ target_link_libraries(
        if (MSVC)
            target_link_libraries(libslic3r_gui PUBLIC Setupapi.lib)
        elseif (CMAKE_SYSTEM_NAME STREQUAL "Linux")
       -    target_link_libraries(libslic3r_gui PUBLIC ''${DBUS_LIBRARIES})
       +    target_link_libraries(libslic3r_gui PUBLIC ''${DBus1_LIBRARIES})
        elseif (APPLE)
            target_link_libraries(libslic3r_gui PUBLIC ''${DISKARBITRATION_LIBRARY} ''${COREWLAN_LIBRARY})
        endif()
       '')
  ];
})

(For reference, the only additional patch needed for 2.9.0-alpha1 is this

(prev.writeText "boost-filesystem.patch" ''
       --- a/src/slic3r/GUI/BackgroundSlicingProcess.cpp
       +++ b/src/slic3r/GUI/BackgroundSlicingProcess.cpp
       @@ -120,7 +120,7 @@ BackgroundSlicingProcess::~BackgroundSlicingProcess()
               std::string prefix = boost::filesystem::path(m_temp_output_path).filename().string();
               prefix = prefix.substr(0, prefix.find('_'));
            for (const auto& entry : boost::filesystem::directory_iterator(temp_dir)) {
       -        if (entry.is_regular_file()) {
       +        if (boost::filesystem::is_regular_file(entry.path())) {
                    const std::string filename = entry.path().filename().string();
                    if (boost::starts_with(filename, prefix) && boost::ends_with(filename, ".gcode"))
                        boost::filesystem::remove(entry);
    '')

)

One nitpick for discussion, should a derivation for opencascade-occt-7_6_1 be created in nixpkgs rather than overriding it inline here? I think it makes sense, since there is precedent for pinned versions already (i.e. opencascade-occt_7_6 which is currently 7.6.2 vs opencascade-occt which is currently 7.8.1).

@jmickelin
Copy link

jmickelin commented Nov 23, 2024

Reading through your patch, it seems you arrived at pretty much the same conclusions, although we solved them differently: Disabling the broken FindEigen3 module, adding the right include directories for dbus-1 and fixing the linking issues with boost, and then linking to OpenSSL as per the patches in the previous version.

For the boost one, note that using BOOST_LOG_DYN_LINK just happens to solve the error by chance (I think because it's deferring the compile-time linking to runtime, where the .so is available), and that the main cause is that CMake just isn't aware that the libraries are needed in some of the files. For the record, Nix has an easier method to define such flags: Rather than patching the CMake files, you can add it to cmakeFlags in the derivation:

  cmakeFlags = ... ++ [ "-DCMAKE_CXX_FLAGS=-DBOOST_LOG_DYN_LINK" ];

I also suggest we turn off building of tests:

  cmakeFlags = ... ++ [ "-DSLIC3R_BUILD_TESTS=0" ];

This is what the derivation for e.g. bambu-studio is doing: https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/misc/bambu-studio/default.nix#L156

@cbat98
Copy link
Author

cbat98 commented Nov 23, 2024

@jmickelin Wow, thank you for such an in-depth comment! I'm a fairly new NixOS user, and haven't really worked with C++ build processes before, so attempting to hack together this update really felt like I was biting off more than I could chew. But anyway, lots of frantic Googling later, I seem to stumbled into a combination of things that have sort-of worked.

I appreciate your pick-up of the fact that I've just overridden the OCCT version inline. I've basically just copied over what I had in a local overlay into the nix-pkgs repo, so that's my bad.

I'll work through my patch file and the overlay that you've provided to come up with a bit of a better fix over the next day or so.

Thanks once again :)

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.

2 participants