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

Mingw cross master #645

Closed
wants to merge 4 commits into from
Closed

Mingw cross master #645

wants to merge 4 commits into from

Conversation

clanmills
Copy link
Collaborator

No description provided.

src/futils.cpp Outdated Show resolved Hide resolved
Copy link
Member

@D4N D4N left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have put 3 quite large binary files into the repo, but these are distributed files from mingw, they imho don't belong into the git repo. I would suggest to fetch these in a script or advise the user where they can download them.

@clanmills
Copy link
Collaborator Author

You're right. I shouldn't have done that.

I did this because I was exhausted with Gilles/MXE bullshit and didn't have the energy to solve the puzzle of how to get them an easier way. Gilles/MXE stuff was building for 24 hours. Having put them in the bundle, I stopped thinking about them.

However, you're right. I should document how to get them and that will resolve how to obtain the 32 bit DLLs. I'll investigate/fix that tomorrow.

I tried to fix the typo concerning MSC_VER in the branch mingw-cross-master. Git refuses to allow me to submit that change.

@codecov
Copy link

codecov bot commented Jan 27, 2019

Codecov Report

Merging #645 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #645   +/-   ##
=======================================
  Coverage   62.24%   62.24%           
=======================================
  Files         154      154           
  Lines       21234    21234           
=======================================
  Hits        13218    13218           
  Misses       8016     8016
Impacted Files Coverage Δ
src/futils.cpp 86.88% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd6332a...e89bfe6. Read the comment docs.

@kamgha
Copy link

kamgha commented Feb 6, 2019

You have put 3 quite large binary files into the repo, but these are distributed files from mingw, they imho don't belong into the git repo. I would suggest to fetch these in a script or advise the user where they can download them.

Hi, I'd suggest to compile in the depedencies statically into the resulting binaries, e. g. through -static or -lstdc++ -lgcc or -static-libstdc++ -static-libgcc.

@clanmills
Copy link
Collaborator Author

Yes. This is a good idea. I'm on vacation in Florida at the moment. We're planning a "dot" release of Exiv2 v0.27.1 to be released at the end of March. I'll work on this when I get home.

This will be the first release of Exiv2 in which we support/document cross compiling. It's a good idea to support both static and dynamic linking.

@D4N
Copy link
Member

D4N commented Feb 6, 2019

Hi, I'd suggest to compile in the depedencies statically into the resulting binaries, e. g. through -static or -lstdc++ -lgcc or -static-libstdc++ -static-libgcc.

As @clanmills said: the plan is to support dynamic and static linking. My complaint was only concerning the addition of these libraries into the git repository.

@clanmills I modified the commits about a week ago and dropped these binaries.

@D4N D4N mentioned this pull request Feb 6, 2019
@piponazo
Copy link
Collaborator

piponazo commented Feb 7, 2019

I did not review yet the code because I see there are some conflicts. I'll review it once the branch is in a "clean" state

@piponazo
Copy link
Collaborator

piponazo commented Feb 10, 2019

It seems we have some warnings (treated as errors on windows):

/Fdsrc\CMakeFiles\exiv2lib.dir\exiv2lib.pdb /FS -c ..\src\basicio.cpp
..\src\basicio.cpp(246): error C2220: warning treated as error - no 'object' file generated
..\src\basicio.cpp(246): warning C4244: '=': conversion from '__int64' to 'off_t', possible loss of data
[43/182] Building CXX object src\CMakeFiles\exiv2lib.dir\bmpimage.cpp.obj
[44/182] Building CXX object src\CMakeFiles\exiv2lib.dir\bigtiffimage.cpp.obj

Plus other things:

..\src\image.cpp(881): fatal error C1017: invalid integer constant expression

@D4N
Copy link
Member

D4N commented Feb 10, 2019

I just dropped the commit that enabled wchar_t on Windows, so they'll go away. We should enable that for #685, but not in this PR.

@@ -610,4 +618,4 @@ $ cmake -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_ENABLE_SSH=ON

[TOC](#TOC)

Written by Robin Mills<br>[email protected]<br>Updated: 2018-11-22
Written by Robin Mills<br>[email protected]<br>Updated: 2018-01-09
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you want to update also the year 😉

-DCMAKE_BUILD_TYPE=Release"


ZLIB=$( realpath ${PWD}/../../../zlib-1.2.11)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something I do not understand about the zlb and expat dependencies. There is one point in which you mentioned that those are handled by conan, and afterwards I understood that we need to compile them by hand?

Are not these libraries handled by conan properly ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I see that you are installing the package mingw64-zlib in that fedora image.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Fedora build does not use this script.

I am not sure if we even should include it at all? It builds a lot of dependencies manually, which is imho not the way to go. We can pull in the mingw dependencies via dnf or apt on Fedora, Debian and Ubuntu. Or probably via Conan on the other platforms.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the Linux distributions we are testing with Gitlab it makes more sense to use the system packages if possible, so that we really tests if those distributions can use Exiv2 without caring much about external dependencies.

Maybe for cross-compiling, it is more difficult to find the system packages ... and we could end up compiling something via a script or conan.

The main aim of my comment was to highlight the contradiction in the documentation.

cp ../gcc64/*.dll bin/
wine bin/exiv2 -vV

# That's all Folks!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤣 do you also love that file ending?

const size_t idxLastSeparator = ret.find_last_of('/');
#endif

const size_t idxLastSeparator = ret.find_last_of(EXV_SEPARATOR_STR);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@piponazo piponazo mentioned this pull request Feb 27, 2019
@D4N D4N mentioned this pull request Feb 27, 2019
@D4N
Copy link
Member

D4N commented Mar 21, 2019

@clanmills do you want to pursue this PR further and add the script for MXE building to the repo?

@clanmills
Copy link
Collaborator Author

I believe you and Luis have put in a lot of effort into this topic and I'm happy to close this. I hope we'll get the new hosting for exiv2.org working this week and I'll release v0.27.1 RC1. I intend to add notes to README-CONAN.md about cross-compiling. I hope it only involves copy/merging from master.

@D4N D4N closed this Mar 25, 2019
@clanmills clanmills deleted the mingw-cross-master branch April 28, 2020 15:16
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.

4 participants