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

windows: fix build by msys2 toolchain #355

Closed
wants to merge 1 commit into from

Conversation

msink
Copy link
Contributor

@msink msink commented May 2, 2018

tested with CMake versions 3.11.0 and 3.1.0
with 3.11.0 - build passed
with 3.1.0 - this part works, but build failed because of empty $<TARGET_PROPERTY:libui,BINARY_DIR>

@andlabs
Copy link
Owner

andlabs commented May 2, 2018

Please remove the appveyor stuff.

As I mentioned in the other issue, I still need an explanation of why this change makes things behave differently. IIRC I had no problems with my own MSYS build a few days ago (though that was from within MSYS using -G"MSYS Makefiles").

@msink
Copy link
Contributor Author

msink commented May 2, 2018

Ok, appveyor removed.
But why? You don't want CI for Windows?

About explanation - I do not understand, different to what?
This works - "C:\Program Files\CMake\bin\cmake.exe" -E copy F:/src/libui/build/CMakeFiles/libui.dir/windows/resources.rc.obj F:/src/libui/build/out/libui.res
This dont - "C:\Program Files\CMake\bin\cmake.exe" -E copy F:/src/libui/build/CMakeFiles/libui.dir/windows/resources.rc.* F:/src/libui/build/out/libui.res

Don't know why, I'm not expert in CMake.
Guess it never worked.

@msink
Copy link
Contributor Author

msink commented May 2, 2018

Maybe it works in sh because it actually does wildcard expansion before calling cmake -E ...,
Native cmd.exe passes wildcard as-is.

@mischnic
Copy link
Contributor

mischnic commented May 2, 2018

Wildcards aren't supported with cmake -E copy: https://gitlab.kitware.com/cmake/cmake/issues/16791

@andlabs
Copy link
Owner

andlabs commented May 2, 2018

I don't mind having CI for Windows. I asked to remove the AppVeyor file for two reasons:

  1. It is unrelated to the PR it is being included with
  2. Someone else is already working on it (Automating binary attachments to release. #157)

I'll respond to the other comments in a bit, but I've decided to just go ahead and make the changes that would result in removing the cmake code in question at all. They won't interfere with @bcampbell's stuff, so I can do this immediately.

@msink
Copy link
Contributor Author

msink commented May 2, 2018

Ok. Not sure it was completely unrelated - it proved buildability - but ok.

And still - do you want CI on Windows now ?
Just to test that it builds.

That PR is two years old, and things changes.
And - I hope - that man is busy :)

I can prepare separate PR just to test buildability, uploading may be added later.
If yes, what toolchains you want to test?

  • msvc-2017 32/64 bit
  • msvc-2013 32/64 bit
  • msys2-mingw 32/64 bit
  • ... ?

@andlabs
Copy link
Owner

andlabs commented May 3, 2018

Okay, I see the discrepancy in wildcard behavior now. I was suspecting this at this point; thanks for confirming.

I decided to just fix the problem by not having it in the first place: there is no more .res file to copy out or link in. Do things work now?

That PR is old, but it's still being actively worked on. You should coordinate your efforts with @parro-it, so everyone wins and no merges conflict =P And yes, for testing, many possible systems would be nice. But setting up CI is still separate from fixing the build, even if it helped find the break...

@parro-it
Copy link
Contributor

parro-it commented May 3, 2018

@andlabs I remember we already discussed somewhere to split #157 by moving the publishing of the prebuilt binaries to a separate PR.
@msink can you help me with that splitting?
It would also be nice to add tests done with @mischnic https://github.com/mischnic/screenshot-tester (if its ready to use).

See some nice screenshot of this test tool:
image

https://snapshots-mtbpfureqh.now.sh/

@msink
Copy link
Contributor Author

msink commented May 3, 2018

@parro-it Ok, I will try, in separate PR
Closing this as obsolete.

@msink msink closed this May 3, 2018
@msink msink deleted the msys2-build branch May 3, 2018 11:27
@ConsoleTVs
Copy link

I think this goes in the same way... #438

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.

5 participants