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

compatibility with MXE #4824

Merged
merged 4 commits into from
Mar 4, 2016
Merged

Conversation

starius
Copy link
Contributor

@starius starius commented Feb 19, 2016

Source: https://github.com/mxe/mxe/blob/master/plugins/apps/qbittorrent-1-fixes.patch
(Change of winconf.pri was improved, commit "convert includes like <Windows.h> to lowercase" is not needed.)

libboost_system-mgw45-mt-1_47 \
libboost_filesystem-mgw45-mt-1_47 \
libboost_thread-mgw45-mt-1_47
}
Copy link
Member

Choose a reason for hiding this comment

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

Removing this section & lib section in winconf.pri cause error for me:

linking release\qbittorrent.exe
LINK : fatal error LNK1181: cannot open input file 'libtorrent.lib'
jom: C:\Users\VM_win7\Desktop\qBittorrent\src\makefile.release [release\qbittorr
ent.exe] Error 1181

@naikel
Copy link
Contributor

naikel commented Feb 20, 2016

This is NOT how it's supposed to be done.

First there's a PR for that already. Second you don't edit the configure file, since that is autogenerated. You modify the qbittorrent.m4 file.

Second for the libraries: you just can't ditch them. That's crazy. It should be something like this:

diff --git a/winconf-mingw.pri b/winconf-mingw.pri
index 0283d23..465e4cc 100644
--- a/winconf-mingw.pri
+++ b/winconf-mingw.pri
@@ -22,17 +22,17 @@ RC_FILE = qbittorrent_mingw.rc

 # Adapt the lib names/versions accordingly
 CONFIG(debug, debug|release) {
-  LIBS += libtorrent \
-          libboost_system-mgw45-mt-d-1_47 \
-          libboost_filesystem-mgw45-mt-d-1_47 \
-          libboost_thread-mgw45-mt-d-1_47
+  LIBS += libtorrent-rasterbar \
+          libboost_system-mt \
+          libboost_filesystem-mt \
+          libboost_thread_win32-mt
 } else {
-  LIBS += libtorrent \
-          libboost_system-mgw45-mt-1_47 \
-          libboost_filesystem-mgw45-mt-1_47 \
-          libboost_thread-mgw45-mt-1_47
+  LIBS += libtorrent-rasterbar \
+          libboost_system-mt \
+          libboost_filesystem-mt \
+          libboost_thread_win32-mt
 }

 LIBS += libadvapi32 libshell32 libuser32
-LIBS += libcrypto.dll libssl.dll libwsock32 libws2_32 libz libiconv.dll
+LIBS += libcrypto libssl libwsock32 libws2_32 libz libiconv
 LIBS += libpowrprof

Just removing the file versions it works with MXE.

@starius
Copy link
Contributor Author

starius commented Feb 21, 2016

First there's a PR for that already.

It is #4631. I have excluded commit 0365bd1c17aa2bfcc6f264864e6804746cf83d45 (do not check qmake existance).

Second for the libraries: you just can't ditch them. That's crazy. It should be something like this

Fixed.

Note that I removed hardcoded paths to libs like "C:/qBittorrent/boost_1_51_0/stage/lib".

I added QMAKE_LRELEASE to QT_QMAKE command in both configure and configure.ac. However when I run autoconf I get a lot of differences between old configure and new configure. Probably my version of autoconf is different.

@sledgehammer999
Copy link
Member

I don't understand how configure works in this case. It generates a conf.pri file which is only included by unixconf.pri and macxconf.pri. It isn't included by the winconf*.pri files at all.

Also the changes in winconf.pri aren't desired. They serve as a guide (on what to edit) for the newcomers that want to build on windows.

@sledgehammer999
Copy link
Member

Also the changes in winconf.pri aren't desired. They serve as a guide (on what to edit) for the newcomers that want to build on windows.

Maybe comment the paths instead? @qbittorrent/qbittorrent-frequent-contributors is that ok?

@sledgehammer999 sledgehammer999 added OS: Windows Issues specific to Windows OS: Linux Issues specific to Linux distributions labels Feb 21, 2016
@starius
Copy link
Contributor Author

starius commented Feb 22, 2016

Also the changes in winconf.pri aren't desired. They serve as a guide (on what to edit) for the newcomers that want to build on windows.

@sledgehammer999, I have excluded commit 62ec26a307a7502ea8aadb6bca0b4ff445e60810, it is not needed for MXE.

@starius
Copy link
Contributor Author

starius commented Feb 22, 2016

it is not needed for MXE.

Oops, it is needed:

linking release/qbittorrent.exe
/bin/sh: 1: cannot open path-according-to-the-build-options-chosen: No such file

I have commented paths out.

@starius
Copy link
Contributor Author

starius commented Feb 22, 2016

I don't understand how configure works in this case.

Neither do I. I changed it to pass QMAKE_LRELEASE to qmake as a environment variable. Maybe there is a better way to do it.

@Chocobo1
Copy link
Member

Maybe comment the paths instead? @qbittorrent/qbittorrent-frequent-contributors is that ok?

IMO it's better to move these to winconf-msvc.pri instead of comment out.

@starius
Copy link
Contributor Author

starius commented Feb 22, 2016

IMO it's better to move these to winconf-msvc.pri instead of comment out.

Are you sure that nobody uses them with MinGW?

By the way, only one path (C:/qBittorrent/RC_0_16/bin/<path-according-to-the-build-options-chosen>) build compilation errors, because of "<" and ">". I have updated the pull request removing "<" and ">" from the path.

@sledgehammer999
Copy link
Member

IMO it's better to move these to winconf-msvc.pri instead of comment out.

Nope. They can be used when building with mingw too.

@Chocobo1
Copy link
Member

Nope. They can be used when building with mingw too.

Well never mind (I think we are not talking about the same thing) and the commits are different since I checked.

@starius
Copy link
Contributor Author

starius commented Feb 22, 2016

Current version only changes C:/qBittorrent/RC_0_16/bin/<path-according-to-the-build-options-chosen> to C:/qBittorrent/RC_0_16/bin/path-according-to-the-build-options-chosen. Removing or commenting of the section is not needed.

@starius
Copy link
Contributor Author

starius commented Feb 28, 2016

Please review this Pull Request.

@sledgehammer999
Copy link
Member

I'll try to do it tomorrow.

@sledgehammer999 sledgehammer999 added this to the v3.3.4 milestone Feb 29, 2016
@@ -53,7 +53,7 @@ CONFIG(debug, debug|release) {
# Enable backtrace support
CONFIG += strace_win

win32-g++ {
*-g++* {
Copy link
Member

Choose a reason for hiding this comment

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

Will it work if you have win32-g++* instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not documented. According to SO answers, it will.
I don't know who to get or set this string in qmake.

Copy link
Member

Choose a reason for hiding this comment

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

The original string is win32-g++ and you change it to *-g++*. I am talking about changing it to win32-g++* which means any g++ on Windows.
The question is will win32-g++* work for MXE? Can you test it? If it does work then change it to what I suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

win32-g++* works in MXE. The PR was updated.

@sledgehammer999
Copy link
Member

Use git rebase -i HEAD~6 to make the following changes. When you use that command a txt file will be opened with your editor. Read the comments in that file to understand what is happening.

  1. Your last commit is actually overwriting your first. So put the last's commit line above the first. Then in the first commit (now second) change the verb to fixup.
  2. The commits winconf-mingw.pri: remove versions from libraries and winconf-mingw.pri: remove ".dll" from libraries do changes in the same file. The changes are very similar so you need just one commit. So fixup or squash the later commit.

After you're done git push --force origin mxe-compat. This PR will autoupdate and I can merge it. Leave a message so I get notified.

@sledgehammer999
Copy link
Member

I am asking you this because I try to keep a clean and short commit history. (but not an overly compacted one)

Mask "win32-g++" doesn't match MXE.
Mask "win32-g++*" match MXE.
See http://stackoverflow.com/a/14523545
See http://mxe.cc
Path with "<", ">" causes errors when building in MXE.
http://mxe.cc/

Removing or commenting out these hardcoded paths is not
desirable, as they serve as a guide (on what to edit) for
the newcomers that want to build on windows.

See qbittorrent#4824 (comment)
  * remove versions from libraries,
  * remove *.dll from libraries
    (actual name of library files can be "libfoo.a")
@starius
Copy link
Contributor Author

starius commented Mar 3, 2016

Rebased. I haven't done it before to keep changes of the PR clear.

sledgehammer999 added a commit that referenced this pull request Mar 4, 2016
@sledgehammer999 sledgehammer999 merged commit 8b559a8 into qbittorrent:master Mar 4, 2016
@sledgehammer999
Copy link
Member

Thank you.

@starius starius deleted the mxe-compat branch March 4, 2016 23:29
sledgehammer999 pushed a commit that referenced this pull request Mar 5, 2016
Path with "<", ">" causes errors when building in MXE.
http://mxe.cc/

Removing or commenting out these hardcoded paths is not
desirable, as they serve as a guide (on what to edit) for
the newcomers that want to build on windows.

See #4824 (comment)
starius added a commit to LuaAndC/mxe that referenced this pull request Apr 18, 2016
The following patches were included in the upstream:

  * convert includes like <Windows.h> to lowercase
    qbittorrent/qBittorrent#4505
  * fix library list
    qbittorrent/qBittorrent#4824
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS: Linux Issues specific to Linux distributions OS: Windows Issues specific to Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants