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

qt4 compatibility and cleanup #53

Closed
wants to merge 7 commits into from
Closed

qt4 compatibility and cleanup #53

wants to merge 7 commits into from

Conversation

GitMensch
Copy link
Contributor

  • Main.cpp: include qt headers from their include root (including the module directory)
  • ThumbnailProvider.cpp [!NDEBUG]: limit includes to the scope they are used for
  • ThumbnailProvider.cpp (CThumbnailProvider::GetThumbnail) [QT_VERSION < 0x050200]: retrofixed (device somehow lost its dereferencing)

Tested with QT4.8.1 and updated VC2019 using:

set QT_DIR=D:\dev\QtSDK\Desktop\Qt\4.8.1\msvc2010
pushd D:\dev\svg-explorer-extension\SVGThumbnailExtension
mkdir release
cl /nologo Main.cpp ClassFactory.cpp ThumbnailProvider.cpp Registry.cpp ThumbnailProvider.def /Zc:wchar_t- /DNDEBUG /DUNICODE /DSVGTHUMBNAILEXTENSION_LIBRARY /DQT_NO_DEBUG /DQT_SVG_LIB /DQT_WIDGETS_LIB /DQT_WINEXTRAS_LIB /DQT_GUI_LIB /DQT_CORE_LIB /D_WINDLL /I "%QT_DIR%\include" /link /LIBPATH:"%QT_DIR%\lib"  /DLL /SUBSYSTEM:WINDOWS /DEF:ThumbnailProvider.def /MANIFEST:embed /OUT:release\SVGThumbnailExtension.dll shlwapi.lib advapi32.lib QtCore4.lib QtSvg4.lib QtGui4.lib

tibold and others added 4 commits January 3, 2020 22:23
* Main.cpp: include qt headers from their include root (including the module directory)
* ThumbnailProvider.cpp [!NDEBUG]: limit includes to the scope they are used for
* ThumbnailProvider.cpp (CThumbnailProvider::GetThumbnail) [QT_VERSION < 0x050200]: retrofixed (`device` somehow lost its dereferencing)
... and changed appveyor URL to tibold
@GitMensch GitMensch mentioned this pull request Jan 3, 2020
@tibold tibold changed the base branch from master to develop January 3, 2020 23:34
@tibold
Copy link
Owner

tibold commented Jan 3, 2020

The develop branch disappeared at some point. Please use that branch for code changes. ;)

@tibold
Copy link
Owner

tibold commented Jan 3, 2020

@GitMensch, does the Build.ps1 work with Qt4? I haven't tested that.

BTW, are you aware that Qt4 reached its end of life a few years ago?

@GitMensch
Copy link
Contributor Author

BTW, are you aware that Qt4 reached its end of life a few years ago?

Yes, but the cleanup is useful in any case and the size for Qt4 vs. the 10GB shiny new one alone is a reason to stay compatible :-)

The develop branch disappeared at some point. Please use that branch for code changes. ;)

Sure, as long as it is there it will be used.

@GitMensch
Copy link
Contributor Author

GitMensch commented Jan 4, 2020

does the Build.ps1 work with Qt4?

Possibly, I've never tried it but the invocation (both for 2017 from its cmd-prompt and for 2019 from its cmd-prompt) raises a "missing spectre" errror.

powershell -Command .\Build.ps1 -VSVersion 2019 -QtInstallPath D:\dev\QtSDK\Desktop\Qt -QtVersion 4.8.1 -Verbose

Failed to configure Visual Studio: [ERROR:vcvars.bat] Spectre is not installed for 'c++' verify compatibility for
'14.24.28314' with spectre
In D:\dev\svg-explorer-extension\deployment\Utils.psm1:66 Column:13
+             throw "Failed to configure Visual Studio: $_"
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (Failed to confi...4' with spectre:String) [], RuntimeException
    + FullyQualifiedErrorId : Failed to configure Visual Studio: [ERROR:vcvars.bat] Spectre is not installed for 'c++' verify compatibility for '14.24.28314' with spectre

@maphew
Copy link
Collaborator

maphew commented Jan 4, 2020

Note there was a Spectre related change in the PR leading up to v1. I captured the new install instructions in readme.md of #52 Matt tweaks

@GitMensch
Copy link
Contributor Author

... I'll recreate my repo once the coverity branch #54 is merged or rejected and redo the PR on the clean develop branch (or someone does the conflict solving ["theirs full" ) repo-version of the install script is the correct answer]).

@GitMensch
Copy link
Contributor Author

For cleaner history I'll recreate the repository...

@GitMensch
Copy link
Contributor Author

@maphew yay, another 870 MB download for Spectre 2017+2019... on its way. Wouldn't be necessary if we can use a MinGW route, would it? This would be actually the last thing for now I'd try to setup (at least on CI), if there is any interest (main purpose: much less to download to setup the environment, less dependencies for the installer [including no additional dependencies]).

@GitMensch
Copy link
Contributor Author

@maphew added spectre for C++, which seems to be "kind of reasonable" to use. But the error goes on:

Failed to configure Visual Studio: [ERROR:vcvars.bat] Spectre is not installed for 'ATLMFC' verify compatibility for
'14.24.28314' with spectre
In D:\dev\svg-explorer-extension\deployment\Utils.psm1:66 Zeichen:13
+             throw "Failed to configure Visual Studio: $_"
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (Failed to confi...4' with spectre:String) [], RuntimeException
    + FullyQualifiedErrorId : Failed to configure Visual Studio: [ERROR:vcvars.bat] Spectre is not installed for 'ATLMFC' verify compatibility for '14.24.28314' with spectre

As far as I see this repo doesn't use any ATL or MFC components (and therefore it shouldn't be used), or did I miss something on my quick glance?

@tibold
Copy link
Owner

tibold commented Jan 4, 2020

I wouldn't mind supporting MinGW for compilation.

I'd keep the releases on the VC++ stack, but I don't see any harm in supporting other toolchains, especially if it simplifies the development environment.

@tibold tibold mentioned this pull request Jan 4, 2020
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.

3 participants