Page MenuHome

Remove MinGW support
ClosedPublic

Authored by Aaron Carlisle (Blendify) on Apr 24 2017, 1:37 AM.

Diff Detail

Repository
rB Blender

Event Timeline

Aaron Carlisle (Blendify) retitled this revision from MinGW: remove from buildbot to Remove MinGW support.Apr 24 2017, 1:50 AM
  • MinGW: Fix missing ')'
  • Fix missing include
  • MinGW: Fix double ')'
  • Fix compiling warning
source/blender/blenlib/intern/path_util.c
1176–1192

This is causing an error still...

  • MinGW: Add back some needed code
  • MinGW: Fix Missing include
Sergey Sharybin (sergey) requested changes to this revision.Apr 24 2017, 9:35 AM

Generally i'm fine removing configuration which was broken for years and nobody maintains. However, several aspects of the patch doesn't make me happy.

  • There should be error notification in cmake when trying to use MinGW. So whoever is trying to compile Blender will have early notification about unsupported platform.
  • Do not modify FindGflags.cmake. It is coming from other project (Ceres solver) and i want this script to be updated in a simple way (which is copy from Ceres folder without applying any patches),
  • Do not remove MinGW support from Libmv. Libmv is a self-sufficient source base (in it's official repo anyway) and it can be easily compiled with MinGW.
  • Do not remove MinGW from Cycles. While it's questionable whether someone is still building Blender with MinGW, removing this support from Cycles standalone will mean we are dropping support of a compiler which is like 10-30% faster at render time. Getting dependencies for Cycles compiled with MinGW should not be that hard.
This revision now requires changes to proceed.Apr 24 2017, 9:35 AM

Note, if mingw support is removed, CMake should probably exit-early with an error that its not supported. Can be done where platform checking is done.

build_files/cmake/config/blender_full.cmake
65–66

this can be removed too.

build_files/cmake/project_info.py
148

if sys.platform == "win32": should be left in, just raise exception that win32 isn't supported.

Aaron Carlisle (Blendify) edited edge metadata.
  • MinGW: From review: revert edits to Cycles, Libmv, FindGflags
  • MinGW: Adress review points raised in cmake
Aaron Carlisle (Blendify) marked 2 inline comments as done.Apr 25 2017, 8:43 PM
  • MinGW: fix missing typedef
  • MinGW: Fix various errors
Brecht Van Lommel (brecht) requested changes to this revision.Apr 28 2017, 10:52 PM
Brecht Van Lommel (brecht) added inline comments.
build_files/cmake/config/blender_full.cmake
67

This line should not be removed.

build_files/cmake/config/blender_release.cmake
68

Same.

source/blender/blenlib/intern/storage.c
424–425

Only of the two lines should have been kept, st1 and st2 are defined twice now which I would expect to give a compile error.

428–431

Same here, should only call one version of wstat.

This revision now requires changes to proceed.Apr 28 2017, 10:52 PM
Aaron Carlisle (Blendify) edited edge metadata.
  • Merge remote-tracking branch 'refs/remotes/origin/master' into temp-remove-mingw
  • MinGW: Address review

Looks good except that this patch now seems to contain some unrelated changes in openexr_api.cpp, openimageio_api.cpp, bpy_rna.c, project_info.py and master_unpack.py?

I can confirm the changes in new line format for the two python files but the other files do not have unrelated changes.
Phab states that they have large changes but after expanding the files they contain only the needed edits.

Ok, well if you carefully check to not commit unrelated changes in these files I have no further comments.

Cleanup looks all-right, doesn't break any tests, be sure not to commit those new line changes, beyond that lgtm

build_files/cmake/project_info.py
254

looks like this file has new-line changes, could you revert these and double check your newline config is set to native in git?

Aaron Carlisle (Blendify) updated this revision to Diff 8834.EditedMay 23 2017, 10:45 PM
  • Merge remote-tracking branch 'refs/remotes/origin/master' into temp-remove-mingw

This also fixes the EOL issue.

Sergey Sharybin (sergey) requested changes to this revision.May 24 2017, 3:49 PM
Sergey Sharybin (sergey) added inline comments.
build_files/cmake/platform/platform_win32.cmake
115

Should be simply

message(FATAL_ERROR "Compiler is unsupported")

Otherwise [FATAL_ERROR] is treated as a string, not as indication that it is a fatal error.

source/blender/editors/space_file/fsmenu.c
753

Don't leave code which is supposed to be removed.

Or at least make it a TODO with more proper description, and not XXX.

This revision now requires changes to proceed.May 24 2017, 3:49 PM
Aaron Carlisle (Blendify) edited edge metadata.
  • From review:
This revision is now accepted and ready to land.May 25 2017, 12:25 PM
This revision was automatically updated to reflect the committed changes.