Skip to content

Conversation

@tpaviot
Copy link
Owner

@tpaviot tpaviot commented Nov 12, 2024

This PR is a follow up for #1347

@farleyrunkel @Andrej730 @Bill-XU can you please review this PR I cannot test the fix.

Summary by Sourcery

Fix the DLL path issue on Windows by adding a function to initialize OCCT libraries using the OCCT_ESSENTIALS_PATH environment variable. Update the INSTALL.md to provide a detailed build guide for Linux and Windows, including prerequisites, system requirements, and troubleshooting.

Bug Fixes:

  • Fix DLL path issue on Windows by initializing OCCT libraries using the OCCT_ESSENTIALS_PATH environment variable.

Documentation:

  • Revise and expand the INSTALL.md documentation to include a comprehensive build guide for both Linux and Windows, with detailed steps and troubleshooting tips.

@sourcery-ai
Copy link

sourcery-ai bot commented Nov 12, 2024

Reviewer's Guide by Sourcery

This PR implements a fix for Windows DLL loading by adding a new initialization mechanism that adds OpenCascade DLL directories to the Windows DLL search path. The changes also include a major update to the installation documentation with comprehensive guides for both Linux and Windows platforms.

Sequence diagram for Windows DLL initialization

sequenceDiagram
    actor User
    participant Application
    participant OS
    participant OpenCascade

    User->>Application: Start Application
    Application->>OS: Check platform
    alt Windows
        OS->>Application: Platform is Windows
        Application->>OS: Check OCCT_ESSENTIALS_PATH
        alt OCCT_ESSENTIALS_PATH set
            OS->>Application: OCCT_ESSENTIALS_PATH is set
            Application->>OpenCascade: Initialize OCCT libraries
            loop For each DLL in OCCT_ESSENTIALS_PATH
                OpenCascade->>OS: Add DLL directory to search path
            end
        else OCCT_ESSENTIALS_PATH not set
            OS->>Application: OCCT_ESSENTIALS_PATH not set
            Application->>User: Raise AssertionError
        end
    else Not Windows
        OS->>Application: Platform is not Windows
    end
Loading

File-Level Changes

Change Details Files
Added Windows DLL path initialization mechanism
  • Created initialize_occt_libraries function to add OpenCascade DLL directories to the search path
  • Added automatic DLL path initialization when running on Windows
  • Added validation of OCCT_ESSENTIALS_PATH environment variable
src/PkgBase/__init__.py
Comprehensive documentation update for installation process
  • Added detailed Windows installation guide
  • Restructured Linux installation guide with clear sections
  • Added troubleshooting section for common issues
  • Added table of contents for better navigation
  • Updated system requirements and prerequisites for both platforms
INSTALL.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @tpaviot - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link

@Bill-XU Bill-XU left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

There is one mistake after Ln.33.
A "break" is needed to prevent further processing the same directory.
The idea is calling "add_dll_directory" upon a directory once only if there is any DLL in it.

Copy link

@Bill-XU Bill-XU left a comment

Choose a reason for hiding this comment

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

Great THANKS ! It becomes more friendly to windows users.:-D

I have some suggestions.

  • Regarding "System Requirements (Windows)" or "Installing Required Software (Windows)"
    rapidjson is also necessary for building RWGltf.
    Use git clone https://github.com/Tencent/rapidjson.git

  • Regarding "Building pythonOCC (Windows) 3. Configure with CMake"
    If using CMake GUI, it is necessary not forgetting add two environment variables in GUI.
    OCCT_INCLUDE_DIR=C:\OpenCASCADE-7.8.1\inc
    OCCT_LIBRARY_DIR=C:\OpenCASCADE-7.8.1\win64\vc14\lib

  • There is no explanation on how to configure rapidjson for RWGltf.
    Here is what I am doing.

    1. Run VisualStudio 2022 Community as administrator
    2. Open PYTHONOCC.sln generated by CMake
    3. Open properties window of project RWGltf
    4. Choose C/C++ on the left
    5. Edit "Additional Include Directories"
    6. Add include directory of rapidjson.
      Note: the include directory of rapidjson is the "include" itself under rapidjson root, and in it there is only one subfolder and its name is "rapidjson". In case you cloned rapidjson to C:\rapidjson, the include directory shall be "C:\rapidjson\include".
    7. If you continue building within VisualStudio, change target to Release, and build the whole solution then the project INSTALL.

    Unfortunately, I only use VisualStudio 2022 Community, so don't know how to make the same changes when using cmake --build.

  • Regarding the value of "OCCT_ESSENTIALS_PATH", I suggest using the root directory of OpenCascade and its 3rd party libraries. (Maybe changing the name to "OCCT_ESSENTIALS_ROOT" is better?)
    In my case, I created a folder named "OpenCASCADE-7.8.1" under "C:\Program Files (x86)". And I extracted all 3rd party libraries in it, also created a folder named "opencascade-7.8.1" for holding compiled OCCT. So I set the value to "C:\Program Files (x86)\OpenCASCADE-7.8.1".
    This folder structure is also recommended by OCCT in this instruction https://dev.opencascade.org/doc/overview/html/index.html#intro_install_windows
    (If the value was "%CASROOT%\win64\vc14\bin", the 3rd party libraries, like TCL/TK DLLs, would not be loaded when the folder structure was as the same as OCCT recommended)

@tpaviot
Copy link
Owner Author

tpaviot commented Nov 12, 2024

@Bill-XU thank you for your comments. For anything related to documentation on building occt/pythonocc for windows, you can open a new issue/pr.

@Andrej730
Copy link
Contributor

I have a suggestion to include setup of OCCT_ESSENTIALS_PATH in cmake on windows, so it will be set up automatically during the pythonocc installation.

@tpaviot
Copy link
Owner Author

tpaviot commented Nov 12, 2024

@Andrej730 You mean the OCCT_ESSENTIALS_PATH would be hard coded after the build is completed?

@Andrej730
Copy link
Contributor

@Andrej730 You mean the OCCT_ESSENTIALS_PATH would be hard coded after the build is completed?

Yeah, I think, this is what most users would expect if build succeeded and Python package is installed - Python package to work without setting up additional environment variable manually.

@tpaviot
Copy link
Owner Author

tpaviot commented Nov 12, 2024

ok that makes sense I will add this feature

set the ```OCCT_ESSENTIALS_ROOT``` environment variable
```batch
setx OCCT_ESSENTIALS_PATH "%CASROOT%\win64\vc14\bin"
setx OCCT_ESSENTIALS_ROOT "%CASROOT%\win64\vc14\bin"
Copy link

Choose a reason for hiding this comment

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

The "OCCT_ESSENTIALS_ROOT" must be the root of OpenCASCADE in which there are not only opencascade itself, but also its 3rd party libraries. Otherwise, many packages (AIS etc.) would throw DLL not found error.
The recommended folder structure is described in OCCT's documentation here https://dev.opencascade.org/doc/overview/html/index.html#intro_install_windows

Copy link

@Bill-XU Bill-XU left a comment

Choose a reason for hiding this comment

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

Sorry, changing Ln.335 is not a suggestion but a correction.
Please check my inline comment and image below, thank you!

20241112214737

@tpaviot
Copy link
Owner Author

tpaviot commented Nov 12, 2024

I'm sorry @Bill-XU I don't get the point between the _ROOT, _PATHS, "root of opencascade" etc. this is specific windows stuff. You can let the OCCT_ESSENTIALS_ROOT env var point wherever needed. If you need a different behavior please submit a patch.

@Bill-XU
Copy link

Bill-XU commented Nov 12, 2024

I'm sorry @Bill-XU I don't get the point between the _ROOT, _PATHS, "root of opencascade" etc. this is specific windows stuff. You can let the OCCT_ESSENTIALS_ROOT env var point wherever needed. If you need a different behavior please submit a patch.

@tpaviot
Err... my apologize for not saying it clearly.
The point is that, the value of the env var should not be "%CASROOT%\win64\vc14\bin", inside which there are only OpenCascade DLLs.
As @Andrej730 mentioned in #1347, it is necessary to load not only opencascade DLLs but also 3rd party libraries.

@tpaviot
Copy link
Owner Author

tpaviot commented Nov 14, 2024

@Andrej730 I tweaked CmakeList.txt so it becomes possible to define OCCT_ESSTNEIALS_ROOT at build time and hard code the related path to config.py. It can however still be defined at run time. Can you please test and report any issue?

@Bill-XU I think the INSTALL.md is now more accurate, can you please review.

@tpaviot
Copy link
Owner Author

tpaviot commented Nov 14, 2024

BTW @Andrej730 I remember you discussed a pythonocc issue, related to shape serialization and version upgrade, but I can't find it anymore, can you please point me to the right place?

@Andrej730
Copy link
Contributor

I remember you discussed a pythonocc issue, related to shape serialization and version upgrade, but I can't find it anymore, can you please point me to the right place?

@tpaviot I believe this is the one - #1350 (comment)

@tpaviot tpaviot merged commit d1dd424 into master Nov 15, 2024
@tpaviot tpaviot deleted the review/fix-issue-1347 branch November 15, 2024 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants