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

[ITK-v5.3.2] Finally fixed windows minc error #10515

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

divital-coder
Copy link
Contributor

@divital-coder divital-coder commented Feb 11, 2025

Hello, So after weeks of head butting, I finally realised the minc error is only and only resolved if we standalone copy the libitkminc2 dll files in the bin directory for windows platforms.

Hoping to get this merged, this is the last time , since i tested it and it works like a charm with the upstream package.

Also, changed the package version to ensure compat bounds.

I tried everything but the only solution for libitkminc2 error in Cmake configuration on windows was copying both the dll files within the
$prefix/bin directory.

Comment on lines +52 to +53
cp $prefix/lib/libitkminc2-5.3.dll $prefix/bin
cp $prefix/lib/libitkminc2-5.3.dll.a $prefix/bin
Copy link
Member

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 many times I need to repeat that these files should be moved, not copied, you just create useless duplicates. And I still haven't understood what's the problem with the import library being in ${prefix}/lib which is where it is for all the other thousands of packages we have here.

Copy link
Contributor Author

@divital-coder divital-coder Feb 12, 2025

Choose a reason for hiding this comment

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

Yes i tried moving them and whatnot, I acknowledge your requests, the CMake config failed at each point.
So this libitkminc is a third party library that is used in ITK, and the Cmake config error, is doing some weird checks.

I tried everything, moving all dll files, moving itkminc dll files, moving them in libdir, moving them in bin
nothing worked. I only succeed upon copying them.

Copy link
Member

Choose a reason for hiding this comment

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

So this libitkminc is a third party library that is used in ITK, and the Cmake config error, is doing some weird checks.

Maybe that should be fixed instead of moving files around without a clear strategy?

Copy link
Contributor Author

@divital-coder divital-coder Feb 12, 2025

Choose a reason for hiding this comment

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

This is only for windows, and I believe this sort of little workaround is really the sufficient thing. I have previously opened issues on the ITK forum, where they mentioned that minc is notorious for errors.

I acknowledge your effort and support in ITK maintenance, I really do :), without your help I wouldn't have been able to catch up to this point.

Copy link
Member

Choose a reason for hiding this comment

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

General suggestion (not just specific to this case): add a comments to explain why you're doing things, especially if they're surprising, hard to understand, and easy to forget for reviewers. I'm 100% sure that next time you touch this code I'll be making the same remarks all over again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for that, get carried away when things work right and just want to get over them, I will precisely add related comments, Thank you so much for raising this concern, and you are right. next time when this thing need to be upgraded, I would have probably gone over the stuff once again.

@giordano giordano enabled auto-merge (squash) February 12, 2025 00:50
@giordano giordano merged commit 52fe346 into JuliaPackaging:master Feb 12, 2025
23 checks passed
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