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

Fix missing include dir for untwine #59169

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Fix missing include dir for untwine #59169

merged 2 commits into from
Oct 23, 2024

Conversation

m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented Oct 22, 2024

Fixes the broken windows build.
Followup #59160

CC @uclaros

@github-actions github-actions bot added this to the 3.40.0 milestone Oct 22, 2024
@uclaros
Copy link
Contributor

uclaros commented Oct 22, 2024

Does it actually fix it? FatalError.hpp is in /external/untwine/untwine/ which is already in the include dirs

@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 22, 2024

Just deducing from the error msg in #59160 (comment), tests will tell

@uclaros
Copy link
Contributor

uclaros commented Oct 22, 2024

Ah, I see, the file is included using it's parent folder too!

Copy link

github-actions bot commented Oct 22, 2024

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 0215bd0)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 0215bd0)

@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 22, 2024

@uclaros there are more issues (about wchar_t/unicode on windows)

Could you have a look at that?

D:\a\QGIS\QGIS\external\untwine\api\QgisUntwine_win.cpp(45): error C2664: 'BOOL CreateProcessA(LPCSTR,LPSTR,LPSECURITY_ATTRIBUTES,LPSECURITY_ATTRIBUTES,BOOL,DWORD,LPVOID,LPCSTR,LPSTARTUPINFOA,LPPROCESS_INFORMATION)': cannot convert argument 9 from 'STARTUPINFO *' to 'LPSTARTUPINFOA'
D:\a\QGIS\QGIS\external\untwine\api\QgisUntwine_win.cpp(52): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or parenthesized function-style cast
C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\um\processthreadsapi.h(369): note: see declaration of 'CreateProcessA'
D:\a\QGIS\QGIS\external\untwine\api\QgisUntwine_win.cpp(45): note: while trying to match the argument list '(const _Elem *, _Ty *, int, int, int, int, int, int, STARTUPINFO *, PROCESS_INFORMATION *)'
        with
        [
            _Elem=char
        ]
        and
        [
            _Ty=char
        ]

@uclaros
Copy link
Contributor

uclaros commented Oct 22, 2024

It looks like I reverted a change from #58563 by pulling the latest untwine (we should have pushed that change to upstream untwine I guess).

Can you try changing again

STARTUPINFO startupInfo;

to

 STARTUPINFOA startupInfo; 

?

@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 22, 2024

Yes, exactly, feel free to push to this PR until it's green or start a new one and merge this.

@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 22, 2024

I think if we wouldn't vendor untwine and only keep the QGIS specific code, we'd be in a much better situation with respect to upstreaming things :-)

@m-kuhn m-kuhn merged commit 0a36a1b into master Oct 23, 2024
29 checks passed
@m-kuhn m-kuhn deleted the fix_untwine_include_dir branch October 23, 2024 09:54
@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 23, 2024

Thanks looks like we are green again. Let's hope tests are working for the next version bump, so we are prepared.

@uclaros
Copy link
Contributor

uclaros commented Oct 23, 2024

I'll push this change to upstream untwine so we're ok next time.
Any changes to our vendored copies should be somehow synced to their upstreams though, or we'll be dealing with such issues (I see #58563 also modified MDAL).

I think if we wouldn't vendor untwine and only keep the QGIS specific code, we'd be in a much better situation with respect to upstreaming things :-)

Unfortunately, there are no packages for untwine, nor lazperf that we could use.

@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 23, 2024

I'll push this change to upstream untwine so we're ok next time.

Thanks

#58563 you will see that it refs multiple previous iterations, with the vendored copy the path of this two-way-sync is not easy (i.e. we don't keep patches, we have a copy of the code).

Unfortunately, there are no packages for untwine, nor lazperf that we could use.

I have tried to create packages and failed because of the architecture, see #57414 (comment) . As long as the code is like this, more packaging is unlikely to happen.

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