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.

Sign up for GitHub

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

Jump to bottom

Upstream packaging.diff #6045

Merged
merged 6 commits into from
Sep 27, 2017
Merged

Upstream packaging.diff #6045

merged 6 commits into from
Sep 27, 2017

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Sep 22, 2017

Please review and test. The admin removal looks dubious to me! See #5957

@ckamm ckamm added this to the 2.4.0 milestone Sep 22, 2017
@ckamm ckamm self-assigned this Sep 22, 2017
dschmidt
dschmidt requested changes Sep 22, 2017
View reviewed changes
Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this crappy task ❤️

src/libsync/theme.cpp
stream << "Git revision " << GIT_SHA1 << endl;
#endif
stream << "Using Qt " << qVersion() << ", built against Qt " << QT_VERSION_STR << endl;
stream << "Using '" << QSslSocket::sslLibraryVersionString() << "'" << endl;
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to check for availability of SSL - not sure Qt does not crash here otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked, it should not crash.

Copy link
Member

@dschmidt dschmidt Sep 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What's the output then? (just out of curiosity)

Copy link
Contributor

Choose a reason for hiding this comment

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

nothing: https://code.woboq.org/qt5/qtbase/src/network/ssl/qsslsocket_openssl11.cpp.html#180

arrguably, it is a bit strange to have an empty Using, but this should not happen in practice.

cmake/modules/DefineCompilerFlags.cmake Outdated
@@ -16,7 +16,8 @@ if (${CMAKE_C_COMPILER_ID} MATCHES "(GNU|Clang)")
# cannot be pedantic with sqlite3 directly linked
# FIXME Can we somehow not use those flags for sqlite3.* but use them for the rest of csync?
if (NOT USE_OUR_OWN_SQLITE3)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=gnu99 -pedantic -pedantic-errors")
# -pedantic-errors explodes in src/csync/csync_private.h:166
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=gnu99 -pedantic")
Copy link
Member

Choose a reason for hiding this comment

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

@ogoffart do you think this is still relevant? Or did something got refactored/cleaned up here?

CMakeLists.txt
@@ -243,7 +267,7 @@ configure_file(version.h.in ${CMAKE_CURRENT_BINARY_DIR}/version.h)
if(BUILD_OWNCLOUD_OSX_BUNDLE)
install(FILES sync-exclude.lst DESTINATION ${OWNCLOUD_OSX_BUNDLE}/Contents/Resources/)
configure_file(sync-exclude.lst bin/${OWNCLOUD_OSX_BUNDLE}/Contents/Resources/sync-exclude.lst COPYONLY)
else()
elseif(BUILD_CLIENT)
Copy link
Member

Choose a reason for hiding this comment

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

This is nonsense - don't know how it got into the diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dschmidt Why is it nonsense? Shouldn't the whole preparation for sync-exclude.lst be done for BUILD_CLIENT only?

CMakeLists.txt Outdated
@@ -247,7 +247,6 @@ if(BUILD_CLIENT)
if(NOT BUILD_LIBRARIES_ONLY)
add_subdirectory(doc)
add_subdirectory(doc/dev)
add_subdirectory(admin)
Copy link
Member

Choose a reason for hiding this comment

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

Was this really in the diff? Looks like an accident or something - it looks pretty wrong, don't even know what it would be good for in the linux packages

@dschmidt
Copy link
Member

I took the liberty of adding the RPATH change to your branch/PR, hope that was ok :trollface:

ogoffart
ogoffart suggested changes Sep 22, 2017
View reviewed changes
src/libsync/theme.cpp
stream << "Git revision " << GIT_SHA1 << endl;
#endif
stream << "Using Qt " << qVersion() << ", built against Qt " << QT_VERSION_STR << endl;
stream << "Using '" << QSslSocket::sslLibraryVersionString() << "'" << endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked, it should not crash.

src/cmd/cmd.cpp Outdated
@@ -196,8 +196,7 @@ void help()

void showVersion()
{
const char *binaryName = APPLICATION_EXECUTABLE "cmd";
std::cout << binaryName << " version " << qPrintable(Theme::instance()->version()) << std::endl;
std::cout << qPrintable(Theme::instance()->versionSwitchOutput());
Copy link
Contributor

Choose a reason for hiding this comment

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

-1: Encoding! should be qPrintableUtf8
(the application executable might contains non-ascii)

cmake/modules/DefineCompilerFlags.cmake Outdated
@@ -16,7 +16,8 @@ if (${CMAKE_C_COMPILER_ID} MATCHES "(GNU|Clang)")
# cannot be pedantic with sqlite3 directly linked
# FIXME Can we somehow not use those flags for sqlite3.* but use them for the rest of csync?
if (NOT USE_OUR_OWN_SQLITE3)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=gnu99 -pedantic -pedantic-errors")
# -pedantic-errors explodes in src/csync/csync_private.h:166
Copy link
Contributor

Choose a reason for hiding this comment

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

csync_private.h:166 is a comment for me.
So I guess this comment is already outdated and should be removed.

Maybe pedentic-errors would make sens on a CI, but not when building packages, so it is fine to remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Ooh of course, it was early, when I wrote the comment ... 😄
Yeah, let's just remove it.

CMakeLists.txt Outdated
find_package(Sparkle)
endif(APPLE)
if(BUILD_CLIENT)
find_package(OpenSSL 1.0.0 REQUIRED) # still needed?
Copy link
Contributor

Choose a reason for hiding this comment

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

-1: Not needed

@ckamm
Copy link
Contributor Author

ckamm commented Sep 22, 2017

@dschmidt Sure :)

@ckamm
Move the --version output text to Theme #5957
ccc23c7 
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
@ckamm
Packaging: Change directory for docs #5957
c0f8671 
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
@ckamm
Packaging: Make check for theme.qrc more specific #5957
2b4293e 
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
@ckamm
Packaging: Adjust APPNAME handling #5957
f77bacd 
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
@ckamm
Packaging: Add options for fine-grained builds #5957
e2828a1 
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
@ckamm
Copy link
Contributor Author

ckamm commented Sep 25, 2017

@ogoffart @dschmidt I did the qPrintableUtf8 change, removed the compile flags change, removed the unneeded find_package(OpenSSL).

Open issue: Is the admin directory intentionally not traversed in cmakelists? @guruz maybe?

@guruz
Copy link
Contributor

guruz commented Sep 25, 2017

Open issue: Is the admin directory intentionally not traversed in cmakelists? @guruz maybe?

👎

Indeed, because for macOS it contains important stuff like macdeployqt.py. So it needs to be there actually.

The packaging.diff was applied on Linux only.

@jnweiger opinion?

@guruz
Copy link
Contributor

guruz commented Sep 25, 2017

FYI @hefee @dragotin

jnweiger
jnweiger approved these changes Sep 25, 2017
View reviewed changes
Copy link
Contributor

@jnweiger jnweiger left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me. Thanks for digging through that ugly heap!

the admin folder is unused in linux, so I left it out at some point. It is indeed needed for macos.

I assume to keep it inside
if (BUILD_CLIENT)
if (NOT BUILD_LIBRARIES_ONLY)
would help macos and not harm linux.

I cannot currently run a test with the nightlies, looks like part of the changes are already in master...
We removed many useless subdirs for linux, to achieve a DFSG-free tarball according to #6005
But removing the admin folder via de8ab35 is probably not needed. I suggest change clean_tarball.sh to not remove the admin folder.

@guruz
Copy link
Contributor

guruz commented Sep 25, 2017

But removing the admin folder via de8ab35 is probably not needed

ack

But removing the admin folder via de8ab35 is probably not needed. I suggest change clean_tarball.sh to not remove the admin folder.

clean_tarball.sh was only intended for external linux packagers, not for us doing all kind of stuff

I cannot currently run a test with the nightlies, looks like part of the changes are already in master..

Wait for this PR to get merged and then see that packaging.diff should not be needed anymore?

@ckamm
Copy link
Contributor Author

ckamm commented Sep 27, 2017

@jnweiger @guruz I've removed the commit that skipped the admin/ directory. I'll merge this now. Let's test and get this rolling :)

@ckamm ckamm merged commit a2ce739 into master Sep 27, 2017
@ckamm ckamm deleted the packaging-fix branch September 27, 2017 07:13
ogoffart
ogoffart reviewed Sep 27, 2017
View reviewed changes
src/libsync/theme.cpp
{
QString helpText;
QTextStream stream(&helpText);
stream << appName().toLatin1().constData()
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooops! Encoding (not new in this patch)

src/libsync/theme.cpp
stream << "Git revision " << GIT_SHA1 << endl;
#endif
stream << "Using Qt " << qVersion() << ", built against Qt " << QT_VERSION_STR << endl;
stream << "Using '" << QSslSocket::sslLibraryVersionString() << "'" << endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

nothing: https://code.woboq.org/qt5/qtbase/src/network/ssl/qsslsocket_openssl11.cpp.html#180

arrguably, it is a bit strange to have an empty Using, but this should not happen in practice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dschmidt dschmidt dschmidt requested changes

@ogoffart ogoffart ogoffart requested changes

@jnweiger jnweiger jnweiger approved these changes

@guruz guruz Awaiting requested review from guruz

Assignees

@ckamm ckamm

Labels
None yet
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ckamm @dschmidt @guruz @ogoffart @jnweiger