Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#9586 closed Bug Report (Fixed)

Commit d149a01 (Changed MPUBLIC define) breaks Windows build

Reported by: Lawrence Rust <lvr@…> Owned by: beirdo
Priority: trivial Milestone: 0.25
Component: MythTV - General Version: Master Head
Severity: medium Keywords: MPUBLIC Windows
Cc: Ticket locked: no

Description

commit d149a01811f1976a863937d836570c59fcd04e24 (change the definition of MPUBLIC to support VS) _totally_ breaks the Windows build using gcc (cross compiled and on Windows with MSys).

PLEASE revert this change ASAP as it's simply not possible to design a simple patch to workaround this. It would be prudent to test any revised changes with the Windows packager or the mythbuild script before committing.

Attachments (10)

mythbuild.log.tar.bz2 (19.4 KB) - added by Lawrence Rust <lvr@…> 8 years ago.
mythbuild.log.tar.2.bz2 (30.2 KB) - added by Lawrence Rust <lvr@…> 8 years ago.
mythbuild.log.tar.3.bz2 (30.7 KB) - added by Lawrence Rust <lvr@…> 8 years ago.
mythbuild.log.tar.4.bz2 (2.6 KB) - added by Lawrence Rust <lvr@…> 8 years ago.
mythbuild.log.bz2 (2.8 KB) - added by Lawrence Rust <lvr@…> 8 years ago.
mythbuild.log.2.bz2 (2.7 KB) - added by Lawrence Rust <lvr@…> 8 years ago.
mythbuild.log.3.bz2 (2.7 KB) - added by Lawrence Rust <lvr@…> 8 years ago.
mythbuild.log.4.bz2 (2.7 KB) - added by Lawrence Rust <lvr@…> 8 years ago.
mythbuild.log.5.bz2 (5.5 KB) - added by Lawrence Rust <lvr@…> 8 years ago.
mythbuild.log.6.bz2 (5.2 KB) - added by Lawrence Rust <lvr@…> 8 years ago.

Download all attachments as: .zip

Change History (47)

comment:1 Changed 8 years ago by beirdo

Priority: criticaltrivial

It would be a good idea to be sure to do the following first before considering backing anything out:

  • make distclean
  • remove the entire contents of the installed include/ dir
  • try again

Also, with no error messages or anything to go on, I'd be hesitant to back anything out.

Also: please do not set the priority field.

comment:2 Changed 8 years ago by Lawrence Rust <lvr@…>

I use my mythbuild.sh script to build for Windows which will always rebuild from scratch after a git pull, simply because the patches need to be re-applied.

Build log attached. Note the important line:

../libmythbase/util.cpp:1340: error: function ‘QString& ShellEscape(QString&)’ definition is marked dllimport

which is because libmyth is including sources from libmythbase. Note also the copious warning messages about mismatched dllimport.

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.tar.bz2 added

comment:3 Changed 8 years ago by beirdo

OK, that actually pointed to a missing change from almost a month ago. I committed a fix for that particular error in 87f6a88307299 on master just now. Please try again.

comment:4 Changed 8 years ago by Lawrence Rust <lvr@…>

A step closer, but no cigar. Build log attached. Looks like a problem with libmythtv importing MythRenderD3D9 and D3D9Image from libmythui. Ironic that it's the Windows specific code that's now broken by this Windows specific change.

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.tar.2.bz2 added

comment:5 in reply to:  4 Changed 8 years ago by markk

Replying to Lawrence Rust <lvr@…>:

A step closer, but no cigar. Build log attached. Looks like a problem with libmythtv importing MythRenderD3D9 and D3D9Image from libmythui. Ironic that it's the Windows specific code that's now broken by this Windows specific change.

Presumably mythrender_d3d9.h just needs a sprinkling of MUI_PUBLIC in the right places.

comment:6 Changed 8 years ago by beirdo

Agreed. I sprinkled in what seemed to be missing in both mythrender_d3d9.h and mythpainter_d3d9.h. This is in 4b220b8cc7b9 on master.

comment:7 Changed 8 years ago by Lawrence Rust <lvr@…>

v0.25pre-1204-ga5a2a9c gets further but now fails while linking libmythmetadata. Looks like mythiowrapper in libmythtv needs the same sprinkling of pixie dust ;-) New build log attached

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.tar.3.bz2 added

comment:8 Changed 8 years ago by beirdo

Owner: set to beirdo
Status: newassigned

comment:9 Changed 8 years ago by beirdo

OK, put some into mythiowrapper.h in eb573d29127a.

comment:10 Changed 8 years ago by Lawrence Rust <lvr@…>

A bit further. Now it bombs on building mythavtest, the 1st program, New log attached.

Looks like including version.pro is the problem. With the current implementation, the apps need to link with libmythbase to access myth_source_version etc. Not ideal, as each program should have that info internally.

Don't you get the feeling that this is a fire fighting exercise? In which case we should look at the implementation. For instance, it is not necessary, just desirable, for a Windows program to mark an external function imported from a DLL as dllimport. If it's marked it saves a few bytes and is slightly faster but can complicate builds. A dll export does have to be marked though unless every extern function is exported.

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.tar.4.bz2 added

comment:11 Changed 8 years ago by beirdo

I think we're almost there. I committed a fix for that in f3b2b05994aaa0. It seems to work just fine for me in Linux.

comment:12 Changed 8 years ago by Lawrence Rust <lvr@…>

Another step closer. The attacched log shows 2 link failures to _myth_source_version

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.bz2 added

comment:13 Changed 8 years ago by Stuart Auchterlonie

Same 2 errors on OSX.

Stuart

comment:14 Changed 8 years ago by beirdo

What a mess. Try with 70a157f96c56

comment:15 Changed 8 years ago by Lawrence Rust <lvr@…>

OK, that's fixed but now there's a missing pthread in mythavtest main.cpp. Looks like the omission of pthread has been lurking here for some time. New log attached.

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.2.bz2 added

comment:16 Changed 8 years ago by beirdo

That should be fixed now.

comment:17 Changed 8 years ago by Lawrence Rust <lvr@…>

Another day. another log ;-). This time in mythtv-setup/backendsettings.cpp:111: undefined reference to `_chanlists'. Log attached.

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.3.bz2 added

comment:18 Changed 8 years ago by beirdo

See 52c0835c92.

comment:19 Changed 8 years ago by Lawrence Rust <lvr@…>

A bit closer but now it complains about mythtv-setup/backendsettings.cpp:111: undefined reference to `impchanlists' Build log attached.

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.4.bz2 added

comment:20 Changed 8 years ago by beirdo

hmm. Try now. I think I may have had the extern vs import/export potentially backwards.

comment:21 Changed 8 years ago by Lawrence Rust <lvr@…>

Unfortunately the same result, even with make clean.

comment:22 Changed 8 years ago by beirdo

See d24531b7f38.

comment:23 Changed 8 years ago by Lawrence Rust <lvr@…>

OK, good progress. I built mythtv OK but failed in mythplugins, MythVideo?. Log attached.

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.5.bz2 added

comment:24 Changed 8 years ago by beirdo

In ffd529f1b671f

comment:25 Changed 8 years ago by Lawrence Rust <lvr@…>

That's fixed, but another problem: mythvideo/fileassoc.cpp:41: undefined reference to `FileAssociations::file_association::file_association(). New log attached

Changed 8 years ago by Lawrence Rust <lvr@…>

Attachment: mythbuild.log.6.bz2 added

comment:26 Changed 8 years ago by beirdo

Try 24484fd3109.

comment:27 Changed 8 years ago by Lawrence Rust <lvr@…>

OK, success! It builds, runs and LiveTV, video, music and gallery all appear to work given a very brief check.

Thanks for your perseverance.

I still can't help feeling that the original change should have been tested with a Windows build or x-compile before committing.

comment:28 Changed 8 years ago by beirdo

Resolution: Fixed
Status: assignedclosed

comment:29 Changed 8 years ago by beirdo

Milestone: unknown0.25

comment:30 Changed 8 years ago by Gavin Hurlbut

Add MUI_PUBLIC to some D3D9 classes

It seems that perhaps a few classes here need to be declared to be public after moving them from libmythtv into libmythui.

Refs #9586

Changeset: 4b220b8cc7b9d7d1e9cab87c84317636d88fb73c

comment:31 Changed 8 years ago by Gavin Hurlbut

Add MTV_PUBLIC to a bunch of mythiowrapper protos

It seems that via libmythbluray, we are trying to import some mythiowrapper functions in libmythmeta. I added the public definitions to allow this to behave as expected with symbol visibility and mingw compiling.

Refs #9586

Changeset: eb573d29127a8b2097c02f29edf31e2a65e343f7

comment:32 Changed 8 years ago by Gavin Hurlbut

Another run at getting rid of symbol issues

Change version.cpp -> libs/libmythbase/version.h, which we then include in mythversion.h. Also convert from extern const char * -> #define constants. This will remove all of the symbols. However, it comes at the cost of a slightly slower re-compile as every file that uses the constants will need recompiling. If we wanted to get around that, we can use functions in libmythbase as the access points, such that only one file needs a recompile, and it is buried in a library, but still anything using said library will need relinking.

Can't win, really. Refs #9586

Changeset: 70a157f96c5644510700057cb9437395188c67d9

comment:33 Changed 8 years ago by Gavin Hurlbut

Fix yet another symbol visibility miss

This time in the frequency table not being exported for mythtv-setup to see. Refs #9586

Changeset: 52c0835c9255233ba5ffde5aff447e7168ac6e7c

comment:34 Changed 8 years ago by Gavin Hurlbut

Take 2 on the freqtable symbol vis.

I think I had the magic backwards, perhaps. If this doesn't fix it, I'm not sure how to proceed. Refs #9586

Changeset: f2ed11dcac7f91c1a326c6b6658d3e4bceb3bc3d

comment:35 Changed 8 years ago by Gavin Hurlbut

Rename frequencies.c -> frequencies.cpp

Since the MTV_PUBLIC only applies to C++ source, we effectively were not ever exporting the chanlists[] table at all. The code in the file compiles just fine as C++, so there should be no harm in doing this.

Refs #9586

Changeset: d24531b7f3802ffeb8482eece97d441d4e1063dd

comment:36 Changed 8 years ago by Gavin Hurlbut

Missing META_PUBLIC for VideoMetadata::SortKey?

Seems that subclasses don't inherit the export from the parent classes?

Refs #9586

Changeset: ffd529f1b671f4dce0f1f40350d365f0ce7644fe

comment:37 Changed 8 years ago by Gavin Hurlbut

Added META_PUBLIC to another subclass.

Seems another public statement was missing. As an interesting note, the "struct file_association" should likely be a "class file_association" if it has members that are method functions.

Refs #9586

Changeset: 24484fd3109260eeb583ca699a24967dd41cba4a

Note: See TracTickets for help on using tickets.