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

tcl/tk: upgrade from 8.6.12 -> 8.6.14 #313

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Sep 4, 2024

Attempt to work through some of the failures in #303

@zanieb zanieb force-pushed the zb/tcl-tk-8614 branch 2 times, most recently from 1ccb82a to 6470105 Compare September 4, 2024 18:01
@zanieb

This comment was marked as outdated.

@zanieb
Copy link
Member Author

zanieb commented Sep 4, 2024

With 0bd2a31 we're past that and now failing validation checks with

  error: python/install/DLLs/tcl86t.dll loads illegal library zlib1.dll
  error: python/install/tcl/nmake/x86_64-w64-mingw32-nmakehlp.exe loads illegal library msvcrt.dll

I was hoping the first wouldn't be present in 3.12 due to https://github.com/zanieb/python-build-standalone/blob/zb/tcl-tk-8614/cpython-windows/build.py#L560-L567 and we could just make that patch unconditionally — but it fails on 3.12 with the same error so that doesn't appear relevant. The relevant CPython change is at python/cpython#101307

The latter file was added upstream in https://core.tcl-lang.org/tcltls/info/b7b0bd5a8f8b09e4

@@ -349,6 +349,11 @@ static DARWIN_ALLOWED_DYLIBS: Lazy<Vec<MachOAllowedDylib>> = Lazy::new(|| {
max_compatibility_version: "1.0.0".try_into().unwrap(),
required: true,
},
MachOAllowedDylib {
name: "/System/Library/Frameworks/UniformTypeIdentifiers.framework/Versions/A/UniformTypeIdentifiers".to_string(),
max_compatibility_version: "1.0.0".try_into().unwrap(),
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how to determine this version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Run otool -L on the binary requiring this library. This prints the compatibility version that is embedded in the Mach-O library dependency data.

@zanieb
Copy link
Member Author

zanieb commented Sep 5, 2024

Most of the macOS builds passing now, one last problem, e.g., for (x86_64-apple-darwin, macos-13, cpython-3.8, pgo):

  error: python/install/lib/libpython3.8d.dylib has weak symbol /System/Library/Frameworks/UniformTypeIdentifiers.framework/Versions/A/UniformTypeIdentifiers:_OBJC_CLASS_$_UTType, which is not allowed on Python 3.8

@zanieb zanieb mentioned this pull request Sep 7, 2024
@zanieb
Copy link
Member Author

zanieb commented Sep 7, 2024

@indygreg I'm not entirely sure what to do about the zlib situation on Windows. I'll continue to poke at it, but if you have any suggestions I'd appreciate them. I think I'll probably spin up a Windows machine to debug, it's a bit of a struggle via CI.

@@ -349,6 +349,11 @@ static DARWIN_ALLOWED_DYLIBS: Lazy<Vec<MachOAllowedDylib>> = Lazy::new(|| {
max_compatibility_version: "1.0.0".try_into().unwrap(),
required: true,
},
MachOAllowedDylib {
name: "/System/Library/Frameworks/UniformTypeIdentifiers.framework/Versions/A/UniformTypeIdentifiers".to_string(),
max_compatibility_version: "1.0.0".try_into().unwrap(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Run otool -L on the binary requiring this library. This prints the compatibility version that is embedded in the Mach-O library dependency data.

Comment on lines +424 to +429
try:
static_replace_in_file(
tcltkprops_path, rb"<tclZlibDLLName>zlib1.dll</tclZlibDLLName>", rb""
)
except NoSearchStringError:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is slightly concerning.

Does CPython's build of tcl require a zlib1.dll? If so, removing this reference may mean we're not shipping a zlib1.dll requiring dependency file?

I don't think we have any references to a zlib1.dll in this project. If there is a zlib1.dll dependency here, that would be concerning.

Copy link
Member Author

@zanieb zanieb Sep 8, 2024

Choose a reason for hiding this comment

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

I don't think this patch (97b9448) actually accomplished anything. I'm just poking around here.

The upstream pull request upgrading the version (python/cpython#101307) says:

I don't see an easy way to avoid having a second copy of zlib specifically for Tcl, so I guess we just have to include it, but I'm pretty sure that's all handled here.

I'm not sure how to make sure this is linked to our zlib. Or if we can just exclude it entirely somehow? The Windows distribution validation is failing with with:

error: python/install/DLLs/tcl86t.dll loads illegal library zlib1.dll

"tcl8",
"tcl8.6",
"thread2.8.7",
"thread2.8.79",

Choose a reason for hiding this comment

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

Looks like a typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, thanks. I'll double check that.

@zanieb zanieb added the dependencies Pull requests that update a dependency file label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants