Code cleanup; Using QCommandLineParser; Exclude list as resource

[hipersayanX]

Also added commons.pri to keep source directory more clean. Modifications here may not break compatibility with older versions of linuxdeployqt.

[probonopd]

Thanks, so you think you could solve the merge conflicts?

[probonopd]

Thanks, will merge once we have fixed #108 (@TheAssassin is working on it)

[Skycoder42]

With this change, #124 becomes unnecessary, because thanks to commons.pri, it can be configured the same. As long as the project is not part of the official Qt, I think this pull request provides the better solution

[hipersayanX]

@probonopd that bug seems quite tricky, I did tried some things but no luck.

[probonopd]

@hipersayanX as a result of us working to fix #108 there is now a merge conflict. May I ask you to resolve it so that this can be merged? Thanks.

[hipersayanX]

@probonopd, done!

[TheAssassin]

First of all, thanks for your contribution. However I found several issues that must be fixed before considering a merge.

The extra lines in `.gitignore are not necessary, they are actually annoying. You shouldn't break upstream's conventions unless discussing this with the team inbefore.
Your so-called cleanups are basically some auto reformats. That makes it super hard to see whether you made changes to the reformatted files for example.

You reformatted the following files in a non-standard way, for no obvious reason:

• shared/shared.h
• shared/shared.cpp

The coredumps have been enabled in the script already, your change to travis.yml is no longer necessary

In tests/tests-ci.sh, you changed the indentation for no real reason.

You do not have to and actually should not include excludelist in this repository.

Rule of thumb: One does not break code style in the middle of a PR. One does not do that at all without discussing that before. Otherwise, PRs will almost always have to be rejected.

[TheAssassin]

Also, please note that your PR is overloaded. Another rule of thumb: One PR per feature, one commit per change. That results in a good structure and improves the abilities to collaborate.

[hipersayanX]

Well, when I started to look in the source code and the first thing I see was when building linuxdeployqt, it always created the linuxdeployqt/linuxdeployqt that was a file that is not part of the source code, but a binary, so I wanted to get rid of that file by adding a rule to .gitignore to ignore it, but the folder and the file had the same name, and the folder was part of the source code, so restructuring the source layout was necessary.

Then I added commons.pri as a way of organizing everything, sending almost all build generated files to folder that can be ignored the leave the project directory clean, and also added the rules for installing the executable in a common path, and added some other common paths for future usages, and all paths can be configured by the user easily.

Then moved all source code from linuxdeployqt and shared folders to a common folder (src), so it follow a the most commonly used conventions:

• http://hiltmon.com/blog/2013/07/03/a-simple-c-plus-plus-project-structure/
• https://gcc.gnu.org/onlinedocs/libstdc++/manual/source_organization.html
• https://softwareengineering.stackexchange.com/questions/81899/how-should-i-organize-my-source-tree/81945#81945
• https://github.com/qtproject/qt-creator

I can cite even more, but there is a lot of C++ projects using the same source layout. So, after all of that I can finally ignore linuxdeployqt, and use git add . without removing that file manually every time.

Also there where no reason for linuxdeployqt/linuxdeployqt.pro and linuxdeployqt.pro to exist as a separate entity, so joined into one unique file, and also because shared.h where not listed in Qt Creator because it was not added in the .pro file.

And finally, Qt Creator automatically cleaned all blank spaces and fixed tabs automatically, and that sum up all changes for my first commit.

The second commit was about getting rid of manual parsing of the arguments, and let Qt parse them, making it more consistent.

At that point I did not wanted to change anything in the code structure.

[hipersayanX]

The problem came when I wanted to trace were are the external variables defined, from were they come from and how they are used, when I hit the Follow symbol under cursor in Qt Creator, it only redirect me to the extern declaration instead of the original declaration, and then I need to manually search were the variable was declared first, making it really hard to trace the source code.

Also there were no any clear way to know which variable where used publicly and which internally.

There where a lot of declared but never defined anywhere functions, and compiler never complained about it, not even a warning.

After thinking a lot, believe me, a lot of time, decided the best option was to reformat everything to avoid those problems, because 1) it was really necessary and there where no other option, and 2) Because even if the the changes where no accepted it can still be useful in some way.

Finally, removed cleaned the algorithm for searching the icon bundle.

---

Long story short, fixed some bugs out there, integrated some interesting changes from other repositories, by my own because automated build was broken at that time and I did not wanted to wait for it to be fixed, because I said before, the code can still be useful anyway.

[probonopd]

@hipersayanX thanks for working with us on this PR. Next time please send a separate PR for every topic you want to achieve, not one gigantic PR that is almost impossible to understand by looking at the diff.

More detailed review will follow from @TheAssassin soon.

[hipersayanX]

Ups sorry, ok.

[TheAssassin]

Well, when I started to look in the source code and the first thing I see was when building linuxdeployqt, it always created the linuxdeployqt/linuxdeployqt that was a file that is not part of the source code, but a binary, so I wanted to get rid of that file by adding a rule to .gitignore to ignore it, but the folder and the file had the same name, and the folder was part of the source code, so restructuring the source layout was necessary.

That could've easily be done in many ways, either by adding linuxdeployqt/linuxdeployqt (yep, you can include directories) to the .gitignore, or adding a .gitignore containing linuxdeployqt in the linuxdeployqt directory (ignore files in subdirectories are pretty common, e.g. when you want to keep an empty directory, you just create a .gitignore with * inside it). But that doesn't matter any more, as we wanted to change the structure anyway. Maybe that helps you with future projects, though.

Then moved all source code from linuxdeployqt and shared folders to a common folder (src), so it follow a the most commonly used conventions:
[...]
I can cite even more, but there is a lot of C++ projects using the same source layout. So, after all of that I can finally ignore linuxdeployqt, and use git add . without removing that file manually every time.

As a professional C++ developer, I would not say that this is a general convention. You can put it in subdirectories, but it is not mandatory. We've discussed this internally, and we approve your suggestion to move it to src/ in this case.

Well, when I started to look in the source code and the first thing I see was when building linuxdeployqt, it always created the linuxdeployqt/linuxdeployqt that was a file that is not part of the source code, but a binary, so I wanted to get rid of that file by adding a rule to .gitignore to ignore it, but the folder and the file had the same name, and the folder was part of the source code, so restructuring the source layout was necessary.

Also there where no reason for linuxdeployqt/linuxdeployqt.pro and linuxdeployqt.pro to exist as a separate entity, so joined into one unique file, and also because shared.h where not listed in Qt Creator because it was not added in the .pro file.

Well, then that's a problem of Qt Creator IMO, you have the same problems with CMake with some generators like the Visual Studio one. But merging the two files is a good idea, and if adding shared.h to it solves your problem with Qt Creator, that's fine for us.

And finally, Qt Creator automatically cleaned all blank spaces and fixed tabs automatically, and that sum up all changes for my first commit.

The sheer amount of automatically done changes makes it extremely hard, if not impossible, to review this PR in a reasonable amount of time, since so many lines are marked as changed. Most open source projects will not accept PRs like this, because the diff is so large.

Do you have an idea how we could see the essence of your changes without the automated markup changes, so that we can understand what you have changed from the diff easily? It can be done, but unfortunately that is quite some work, which we try to avoid.

I hope you will understand that I cannot merge your PR in the current form. But I do appreciate and would like to see the essence of your changes. If required, we can discuss whether a code style change (for example using the Qt style autocorrection) is desirable afterwards, perhaps in a second PR or a new issue.

The second commit was about getting rid of manual parsing of the arguments, and let Qt parse them, making it more consistent.

That's a good thing, and gets us closer to #84.

---

To sum up: This PR needs to be redone without the automated formatting changes. I'm also dissatisfied of the additional work required for this PR (especially on your side), but be assured that we appreciate any contributions, so thanks so far for the points I already reviewed positively. If you can fix the rest, I'd happily merge your PR.

[hipersayanX]

Sorry for not answering before.

That could've easily be done in many ways, either by adding linuxdeployqt/linuxdeployqt (yep, you can include directories) to the .gitignore, or adding a .gitignore containing linuxdeployqt in the linuxdeployqt directory (ignore files in subdirectories are pretty common, e.g. when you want to keep an empty directory, you just create a .gitignore with * inside it). But that doesn't matter any more, as we wanted to change the structure anyway. Maybe that helps you with future projects, though.

Thanks a lot, honestly I didn't know that was possible, I thought that the filter was valid for files and folders only.

Do you have an idea how we could see the essence of your changes without the automated markup changes, so that we can understand what you have changed from the diff easily? It can be done, but unfortunately that is quite some work, which we try to avoid.

What I normally do is, not sure if that is the best way, is comparing the changes with github, so I did when integrated the latest changes, because the difference was not that much huge (and yes, now I'm figuring out was was the error, in my side).

If you can fix the rest, I'd happily merge your PR.

Sure, any directions from where to start?

[TheAssassin]

I'd say, either revert or nuke (e.g. by rebasing interactively) the commits in which you applied the Qt auto format stuff. Also, please edit the commit that introduced the blank lines between the section headings and the actual entries (or make a new commit). Same goes for the change of indentation in tests-ci.sh, although you'd have to make a new commit for that, since the other changes are still necessary.

Then you should remove the commit in which you added the copy of excludelist. This file changes a lot, and should be fetched from the according repository regularly. This means the qrc file is no longer of any use and can be removed, too.

You can further revert the travis YML configuration, too, as the core dump handling is performed in tests-ci.sh now. You have to update your branch anyway, as it's out of date. You'll see that I've implemented that meanwhile.

I think that covers most of it. I'll check when you're done. You should consult my previous reviews if you think you're done.

If you need further assistance, check out our chat: https://github.com/AppImage/AppImageKit/wiki/Chat#irc.

[TheAssassin]

Since this hasn't been touched in a while, I'd like to ask whether @hipersayanX is still interested in merging this PR. Recently, I was working on this project, and that involved adding a command line parameter. When reading about this PR, I've seen it introduces a QCommandLineParser based CLI, and that'd fix some of the issues I noticed while working on that piece of code.

If you won't get to updating your PR, I'll try to port some of the changes you suggested myself.

[hipersayanX]

@TheAssassin, for me right now, i don't have a use for it since use my own deploy scripts, and right now i have a work overload to fix, so almost impossible to get into this, sorry 😞

[probonopd]

Does this solve a real issue? Otherwise "never touch a running system", since when there is nothing broken, changing it can only make it worse ;-)

[TheAssassin]

@probonopd to be honest, the current main() is totally overloaded, and the program flow is quite weird, which didn't get better with my PR #226.

Sentences like "never touch anything that works" just don't apply in software development, at least when talking about refactoring.

But, if you want some real numbers, just compare AppImageUpdate vs. linuxdeployqt (

linuxdeployqt/tools/linuxdeployqt/main.cpp

Lines 361 to 421 in 002059c

	for (int i = 2; i < argc; ++i) {
	QByteArray argument = QByteArray(argv[i]);
	if (argument == QByteArray("-no-plugins")) {
	LogDebug() << "Argument found:" << argument;
	plugins = false;
	} else if (argument == QByteArray("-appimage")) {
	LogDebug() << "Argument found:" << argument;
	appimage = true;
	bundleAllButCoreLibs = true;
	} else if (argument == QByteArray("-no-strip")) {
	LogDebug() << "Argument found:" << argument;
	runStripEnabled = false;
	} else if (argument == QByteArray("-bundle-non-qt-libs")) {
	LogDebug() << "Argument found:" << argument;
	bundleAllButCoreLibs = true;
	} else if (argument.startsWith(QByteArray("-verbose"))) {
	LogDebug() << "Argument found:" << argument;
	int index = argument.indexOf("=");
	bool ok = false;
	int number = argument.mid(index+1).toInt(&ok);
	if (!ok)
	LogError() << "Could not parse verbose level";
	else
	logLevel = number;
	} else if (argument.startsWith(QByteArray("-executable"))) {
	LogDebug() << "Argument found:" << argument;
	int index = argument.indexOf('=');
	if (index == -1)
	LogError() << "Missing executable path";
	else
	additionalExecutables << argument.mid(index+1);
	} else if (argument.startsWith(QByteArray("-qmldir"))) {
	LogDebug() << "Argument found:" << argument;
	qmldirArgumentUsed = true;
	int index = argument.indexOf('=');
	if (index == -1)
	LogError() << "Missing qml directory path";
	else
	qmlDirs << argument.mid(index+1);
	} else if (argument == QByteArray("-always-overwrite")) {
	LogDebug() << "Argument found:" << argument;
	alwaysOwerwriteEnabled = true;
	} else if (argument.startsWith("-qmake=")) {
	LogDebug() << "Argument found:" << argument;
	int index = argument.indexOf("=");
	qmakeExecutable = argument.mid(index+1);
	} else if (argument == QByteArray("-no-translations")) {
	LogDebug() << "Argument found:" << argument;
	skipTranslations = true;
	} else if (argument.startsWith("-extra-plugins=")) {
	LogDebug() << "Argument found:" << argument;
	int index = argument.indexOf("=");
	extraQtPlugins = QString(argument.mid(index + 1)).split(",");
	} else if (argument.startsWith("-")) {
	LogError() << "Error: arguments must not start with --, only -" << "\n";
	return 1;
	} else {
	LogError() << "Unknown argument" << argument << "\n";
	return 1;
	}
	}

,

linuxdeployqt/tools/linuxdeployqt/main.cpp

Lines 57 to 104 in 002059c

	for (int i = 0; i < argc; i++ ) {
	QString argument = argv[i];
	if (argument == "-version" || argument == "-V" || argument == "--version") {
	// can just exit normally, version has been printed above
	return 0;
	}
	}
	if (argc < 2 || (firstArgument.startsWith("-"))) {
	qInfo() << "";
	qInfo() << "Usage: linuxdeployqt <app-binary|desktop file> [options]";
	qInfo() << "";
	qInfo() << "Options:";
	qInfo() << " -verbose=<0-3> : 0 = no output, 1 = error/warning (default),";
	qInfo() << " 2 = normal, 3 = debug";
	qInfo() << " -no-plugins : Skip plugin deployment";
	qInfo() << " -appimage : Create an AppImage (implies -bundle-non-qt-libs)";
	qInfo() << " -no-strip : Don't run 'strip' on the binaries";
	qInfo() << " -bundle-non-qt-libs : Also bundle non-core, non-Qt libraries";
	qInfo() << " -executable=<path> : Let the given executable use the deployed libraries";
	qInfo() << " too";
	qInfo() << " -qmldir=<path> : Scan for QML imports in the given path";
	qInfo() << " -always-overwrite : Copy files even if the target file exists";
	qInfo() << " -qmake=<path> : The qmake executable to use";
	qInfo() << " -no-translations : Skip deployment of translations.";
	qInfo() << " -extra-plugins=<list> : List of extra plugins which should be deployed,";
	qInfo() << " separated by comma.";
	qInfo() << " -version : Print version statement and exit.";
	qInfo() << "";
	qInfo() << "linuxdeployqt takes an application as input and makes it";
	qInfo() << "self-contained by copying in the Qt libraries and plugins that";
	qInfo() << "the application uses.";
	qInfo() << "";
	qInfo() << "By default it deploys the Qt instance that qmake on the $PATH points to.";
	qInfo() << "The '-qmake' option can be used to point to the qmake executable";
	qInfo() << "to be used instead.";
	qInfo() << "";
	qInfo() << "Plugins related to a Qt library are copied in with the library.";
	/* TODO: To be implemented
	qDebug() << "The accessibility, image formats, and text codec";
	qDebug() << "plugins are always copied, unless \"-no-plugins\" is specified.";
	*/
	qInfo() << "";
	qInfo() << "See the \"Deploying Applications on Linux\" topic in the";
	qInfo() << "documentation for more information about deployment on Linux.";
	return 1;
	}

, and a few more lines scattered across main()).

[probonopd]

I'm just saying, I am doing this in my free time, and if anything is working for me in linuxdeployqt, it's command line parsing ;-)

the program flow is quite weird

I agree, although to my defense I can say that most of this is inherited rather than written from scratch.

[TheAssassin]

Noone's blaming you unless git blame tells us to do so. Although I guess that'd be mostly dominated by @probonopd commits, so it might not be quite fair. But anyway, I'm just trying to improve things, and I found that to be a good starting point.

[hipersayanX]

@probonopd

never touch a running system

Running is not enough when it lack key features.

From my experience, this a list of issues i found in linuxdeployqt:

• It does no considers the case of a heavy modularized application (many executables, plugins, etc.).
• It doesn't considers the case of a complex project tree.
• Libraries that can be included or not in the bundle are hardcoded instead of giving the user the control of what they want and what they don't.
• It doesn't allow custom dependencies rules (ie. rules for solving GStreamer plugins).
• It doesn't consider the case of non-Qt based projects (GTK, SDL, etc.).
• The dependency with Qt is totally unnecessary (unless you use QJSEngine for scripting).

linuxdeployqt is not bad, but it needs a lot of work for packaging a more or less complex application.