Skip to content

Fix arg.name → arg.arg_name in pywrap macros.j2#205

Closed
elliejs wants to merge 1 commit into
CadQuery:masterfrom
elliejs:rename-arg-name-member
Closed

Fix arg.name → arg.arg_name in pywrap macros.j2#205
elliejs wants to merge 1 commit into
CadQuery:masterfrom
elliejs:rename-arg-name-member

Conversation

@elliejs

@elliejs elliejs commented Jul 16, 2026

Copy link
Copy Markdown

Summary

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

Points submodule at elliejs/pywrap 499cd39 which fixes the arg.name →
arg.arg_name bug in macros.j2 (CadQuery/pywrap#63). This is a dummy PR
to verify that the fix builds correctly on all platforms.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@elliejs

elliejs commented Jul 16, 2026

Copy link
Copy Markdown
Author

@adam-urbanczyk I had to wait for my local action to finish up so I could safely delete the generated sources cache. https://github.com/elliejs/OCP/actions/runs/29472183070 this action is an example showing that upstream pywrap actually fails to generate valid sources right now. (yes, there are other commits in that chain, oops, but I didn't want to restart the action run and they don't materially affect the way the crash occurs)

The reason all the CI pipelines keep succeeding with pywrap where it is is because the sources cache just looks for successfully generated sources from any point in history. when pywrap's submodule pointer updates, this doesn't trigger a regeneration of sources in OCP's CI pipeline. I can make a new PR to address the fact that this should probably be true if you agree.

That PR would just add the pywrap short hash to the end of the generated sources artifact, so that when you go looking for eg OCP-src-Windows- in the cache, you have to look for OCP-src-Windows-[pywrap-hash]

This keeps pywrap's head cleaner.

EDIT: I was wrong, PR's run in the upstream's repo context, but CadQuery/OCP appears to have no cache members... I swear it used to, and my fork behaves this way to skip generation phase when possible...

@adam-urbanczyk

Copy link
Copy Markdown
Member

Note that master OCP does not pin master pywrap currently bur rather a different branch (cf. https://github.com/CadQuery/pywrap/tree/f378d84a6a3a0390f72d8128e2619ba73ef7074a). It actually does run fine and it has no caching.

@elliejs

elliejs commented Jul 16, 2026

Copy link
Copy Markdown
Author

thanks, yeah. Talked with Jern and learned that same information. The pinned commit runs fine. head is the problem.

@adam-urbanczyk

Copy link
Copy Markdown
Member

The current situation with branches is somewhat messy. I will rectify it eventually, but I'm working on OCP8 right now. The ask here is to come up with a minimal set of changes that are required for FreeBSD.

@elliejs

elliejs commented Jul 16, 2026

Copy link
Copy Markdown
Author

ok so treat CadQuery/pywrap@f378d84 as head?

@adam-urbanczyk

Copy link
Copy Markdown
Member

I'm not sure what you mean as HEAD in this context. This is the commit used for building OCP7.9 right now. With that being said, does pywrap on this commit actually need any changes for FreeBSD? NB: symbol dumping and the CMake template lives here.

@elliejs

elliejs commented Jul 16, 2026

Copy link
Copy Markdown
Author

HEAD as in where we should be making changes from. Yes, we need one change in translation_unit.py.

I also thought we needed to change the cmakelists in the jinja template so that when it's compiled into the txt form it retains those changes?

@adam-urbanczyk

Copy link
Copy Markdown
Member

Which change in the translation_unit.py is needed? If all is fine, this is the CMake teamplate https://github.com/CadQuery/OCP/blob/master/templates/CMakeLists.j2

@elliejs

elliejs commented Jul 16, 2026

Copy link
Copy Markdown
Author

If you'd like me to base my edits to pywrap off f378d84, you'll need to give it a branch name so the pr can actually work.

@elliejs

elliejs commented Jul 16, 2026

Copy link
Copy Markdown
Author

this is the pywrap commit needed: elliejs/pywrap@7536962

@adam-urbanczyk

Copy link
Copy Markdown
Member

Here is the branch https://github.com/CadQuery/pywrap/tree/OCP79, please adjust the CMakeLists.j2 here and not in the pywrap repo though.

@elliejs

elliejs commented Jul 16, 2026

Copy link
Copy Markdown
Author

I see that OCP has a CMakeLists.j2 too, but in local testing, the actual j2 file that seemed to render was the one in pywrap. That appears to still be true, and this explanation from claude appears to hold water:

  1. cmake invokes pywrap with WORKING_DIRECTORY set to the build dir
    CMakeLists.txt:124 — WORKING_DIRECTORY ${CMAKE_BINARY_DIR} (i.e. /home/plonit/OCP/build)

  2. The toml path is absolute, but template_path inside it is relative
    CMakeLists.txt:123 — passes ${CMAKE_SOURCE_DIR}/ocp.toml (absolute)
    ocp.toml:4 — template_path = "./templates" (relative)

  3. read_settings() loads the toml but doesn't resolve relative paths against the toml's location
    bindgen/init.py:36-39 — read_settings(p) opens p, calls toml.load(f), returns raw settings. The path to the toml file (p) is available here but never
    used to rebase relative paths.

  4. The render function uses template_path as-is, resolved against CWD
    bindgen/init.py:529 — Path(settings["template_path"]) creates Path("./templates")
    bindgen/init.py:531 — placed first in template_paths
    bindgen/init.py:534 — passed to FileSystemLoader(template_paths)

Since CWD is build/, FileSystemLoader looks in build/templates/ — which doesn't exist. It silently falls back to the default path (bindgen/init.py:527
— Path(file).dirname(), i.e. pywrap/bindgen/), and renders pywrap's bundled CMakeLists.j2 instead of OCP/templates/CMakeLists.j2.

This appears to make sense to me. Claude also offered a solution, but I don't want to open another pr without you agreeing that this is the right path forward (instead of eg changing the j2 in pywrap)

Fix options:

The simplest is to resolve template_path relative to the toml file in read_settings():

def read_settings(p):
       with open(p) as f:                                                                                                                                     
           settings = toml.load(f)                                
 
       # resolve template_path relative to the config file, not CWD                                                                                           
       if settings.get("template_path"):
           settings["template_path"] = str(Path(p).parent / settings["template_path"])

This way ./templates becomes /[abspath]/OCP/templates regardless of CWD. It's a one-line fix in pywrap, backwards compatible (absolute paths pass through unchanged), and makes template_path work the way anyone reading ocp.toml would expect

@adam-urbanczyk

Copy link
Copy Markdown
Member

Pls, no more Claude suggested PRs. The release artifacts are clearly generated from the version in this repo (cf. minimum required delta between the two templates). How are you running the generation step? You should be using cmake and the CMakeLists from this repo (see also CI).

@elliejs

elliejs commented Jul 16, 2026

Copy link
Copy Markdown
Author

oh I ran cmake -S . -B build, not -S . -B . like the other platforms.

@elliejs elliejs closed this Jul 17, 2026
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.

2 participants