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

QJournalctl win32 x64 port. #41

Merged
merged 26 commits into from
Mar 28, 2020

Conversation

alacasta
Copy link
Contributor

@alacasta alacasta commented Mar 22, 2020

What?

This is a contribution which aims to port the QJournalctl application to Windows.

Details

The major contribution is

  • To provide an script and updated configuration to be able to build the application for Windows.
  • To add an appveyor configuration to be able to directly obtain the released artifacts from there (deployed artifacts)

There are some few changes applied to the codebase:

  • Migration of the usleep to a multi-platform equivalent call
  • Minor modification from iniializing an array of size a variable, which is not compliant with the msvc build rules. Migrated to safer malloc based initialization.

Also to the README.md

  • Unifying the type of Markdown Titles
  • Reordering the building chapter
  • Adding an specific Windows chapter

Remaining tasks

  • To update the Changelog in case of performing a releease with these changes
  • To integrate appveyor to the official repository.

@alacasta alacasta marked this pull request as ready for review March 22, 2020 21:35
@alacasta alacasta changed the title Port/qjournalct win32 build QJournalct win32 x64 port. Mar 22, 2020
@alacasta alacasta changed the title QJournalct win32 x64 port. QJournalctl win32 x64 port. Mar 22, 2020
@pentix
Copy link
Owner

pentix commented Mar 27, 2020

As already stated in my previous email, I'd suggest some changes: 😌

  • Fixing the "Save SSH connections" function
  • Disabling all local systemd related functions for the Windows port
  • Copy the detailed setup instructions into the README
  • Prefix/Squash all "fix CI" commits together to keep the commit history somewhat structured
  • Maybe some cosmetic changes in the autogen_and_build.bat script

Copy link
Owner

@pentix pentix left a comment

Choose a reason for hiding this comment

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

Just some cosmetic issues actually

REM ---------------------------------------------------------------------
set OLD_PATH=%PATH%
rem "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Auxiliary\Build\vcvarsall.bat" x86
rem set QTDIR=C:\Qt\Qt5.14.1\5.14.1\msvc2017
Copy link
Owner

Choose a reason for hiding this comment

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

Is there an simple method to display the actually executed commands? This could get misleading/confusing if we log version A but execute version B...

set OLD_PATH=%PATH%
rem "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Auxiliary\Build\vcvarsall.bat" x86
rem set QTDIR=C:\Qt\Qt5.14.1\5.14.1\msvc2017
rem "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Auxiliary\Build\vcvarsall.bat" x64
Copy link
Owner

Choose a reason for hiding this comment

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

What about adding call .....\.....\vcvarsall.bat x64 directly for the user? I guess the problem is we would need to find the correct installation path of Visual Studio, right? 😬

rem qmake qjournalctl.pro CONFIG+=release VCPKG_FOLDER=.

rem 64 bits
qmake qjournalctl.pro CONFIG+=release CONFIG+=x86_64 VCPKG_FOLDER=%VCPKG_INSTALL_FOLDER%
Copy link
Owner

@pentix pentix Mar 28, 2020

Choose a reason for hiding this comment

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

Before starting the build, it would be nice to have version.h adjusted just as the autogen.sh script does. (

if [ -d ".git" ]; then
)

However I'm almost sure there's a better (and more portable) way of doing this than what I'm doing right now...
(Maybe qmake itself offers a solution to this?)

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.

None yet

2 participants