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

Qtifw build installer #2966

Merged
merged 5 commits into from Oct 12, 2017

Conversation

Projects
None yet
5 participants
@jroweboy
Copy link
Member

jroweboy commented Sep 28, 2017

Depends on: citra-emu/ext-windows-bin#10

Supercedes: #2929

(Got tired of waiting for @j-selby to get internet back)

This reworks the installer to be built through cmake instead of powershell/bash scripts. I tested windows, but if someone would be so kind as to test macOS and linux.

After this PR, there is one more followup PR for QT gui integration to allow users to update through the UI. When that follow up PR is merged, the download page will need to be reworked as well to point directly to the new installer instead. (We will need lots of testing of course!)


This change is Reviewable

j-selby added some commits Sep 16, 2017

@jroweboy jroweboy force-pushed the jroweboy:qtifw_build_installer branch 3 times, most recently from ba24415 to ddf9f34 Sep 28, 2017

@jroweboy jroweboy force-pushed the jroweboy:qtifw_build_installer branch from ddf9f34 to 709c89a Sep 28, 2017

@B3n30
Copy link
Contributor

B3n30 left a comment

How are we going to generate those installers? CI? How will we upload the installers to the website?


set(CONFIG_FILE "${SRC_DIR}/config/config_${PLATFORM}.xml")
set(INSTALLER_BASE "${BUILD_DIR}/installerbase_${PLATFORM}")
set(BINARY_CREATOR "${BUILD_DIR}/binarycreator_${PLATFORM}")

This comment has been minimized.

@B3n30

B3n30 Sep 28, 2017

Contributor

chmod +x needs to be added to binarycreator_${PLATFORM} (at least on osx)

set(DIST_DIR "${BUILD_DIR}/dist")
set(ARCHIVE "${PLATFORM}.7z")

file(MAKE_DIRECTORY ${DIST_DIR})

This comment has been minimized.

@B3n30

B3n30 Sep 28, 2017

Contributor

file(MAKE_DIRECTORY ${BUILD_DIR}) is needed before

set(PACKAGES_DIR "${BUILD_DIR}/packages")
file(MAKE_DIRECTORY ${PACKAGES_DIR})

execute_process(COMMAND ${BINARY_CREATOR} -t ${INSTALLER_BASE} -n -c ${CONFIG_FILE} -p ${PACKAGES_DIR} ${TARGET_FILE})

This comment has been minimized.

@B3n30

B3n30 Sep 28, 2017

Contributor

chmod +x ${TARGET_FILE}.app/Contents/MacOS/${TARGET_FILE} is also required for osx


Installers can only be built on the platform that they are targeting.

Build the `installer` target to generate the installer, and the installer will be in

This comment has been minimized.

@B3n30

B3n30 Sep 28, 2017

Contributor

Can be in an later PR but this should be better explained depending on the os

This comment has been minimized.

@j-selby

j-selby Sep 29, 2017

Contributor

Don't think it needs to be overly verbose. Direct CMake instructions would be good though.

This comment has been minimized.

@jroweboy

jroweboy Sep 29, 2017

Member

its different for every generator :P not sure i care enough to explain how to build a specific target

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Sep 28, 2017

@B3n30 the installer is an online installer, meaning its only created once and it will read package information from the internet. This means that we won't need to do anything as part of the CI process since its "deploy once use forever (tm)" The only time we'll need to rebuild the installer, is when we upgrade to a newer version of the library (version 3 came out recently actually) and we can build it manually and upload it to the web server like we already have done. The point of this PR is just to make it easy for us to build it again in the future (and have some documentation about how to build it)

Edit: Note that we could change this to generate an offline installer, but for a phase one, i think this is good enough

@j-selby
Copy link
Contributor

j-selby left a comment

How are we going to distribute binaries for Macs? A .app is a directory, no? Would generating a .dmg or another archive be suitable here?

WORKING_DIRECTORY ${BUILD_DIR}
)

add_custom_target(installer DEPENDS ${TARGET_FILE})

This comment has been minimized.

@j-selby

j-selby Sep 29, 2017

Contributor

Newline


Installers can only be built on the platform that they are targeting.

Build the `installer` target to generate the installer, and the installer will be in

This comment has been minimized.

@j-selby

j-selby Sep 29, 2017

Contributor

Don't think it needs to be overly verbose. Direct CMake instructions would be good though.

<Title>Citra Updater</Title>
<Publisher>Citra team</Publisher>
<!-- e.g. /home/<user>/Citra -->
<TargetDir>@HomeDir@/Citra</TargetDir>

This comment has been minimized.

@CodingKoopa

CodingKoopa Sep 29, 2017

Member

I'm not sure if this behavior is really expected? Generally Linux binaries aren't stored in the home directory, in favor of /usr/bin/.

This comment has been minimized.

@j-selby

j-selby Sep 29, 2017

Contributor

The issue here was that QtIFW cannot sudo up by itself (bugged), so it crashed when writing to those directories. To make its behaviour logical, we write to here.

This comment has been minimized.

@jroweboy

jroweboy Oct 11, 2017

Member

We migrated to QTIFW 3.0 which resolves the issue. It also added an option for installing to a different directory if run as root. So now when installing without root, we'll put it in ~/.Citra When installing with root, we'll put it in /opt/Citra

This comment has been minimized.

@chris062689

chris062689 Oct 11, 2017

Member

Do we prompt for root at the start of the installer?

This comment has been minimized.

@jroweboy

@jroweboy jroweboy force-pushed the jroweboy:qtifw_build_installer branch from 11a6c82 to 9e5ba1e Sep 29, 2017

# Adds a custom target that will run the BuildInstaller.cmake file
# CMake can't just run a cmake function as a custom command, so this is a way around it.
# Calls the cmake command and runs a cmake file in "scripting" mode passing in variables with -D
add_custom_command(OUTPUT "${TARGET_FILE}"

This comment has been minimized.

@B3n30

B3n30 Sep 29, 2017

Contributor

the installer dir is needs to exist here. So the make_directory command needs to be executed before that command.

FYI thats the command it tries to execute but fails with the cd
cd /Users/bthomas/citra/build/installer && /Applications/CMake.app/Contents/bin/cmake -DSRC_DIR=/Users/bthomas/citra/dist/installer -D BUILD_DIR=/Users/bthomas/citra/build/installer -D TARGET_FILE=/Users/bthomas/citra/build/installer/dist/citra-setup-mac -P /Users/bthomas/citra/CMakeModules/BuildInstaller.cmake

@jroweboy jroweboy force-pushed the jroweboy:qtifw_build_installer branch 2 times, most recently from 4d83427 to 4234ead Sep 29, 2017

@B3n30

B3n30 approved these changes Sep 29, 2017

Copy link
Contributor

B3n30 left a comment

Tested on osx. Everything is fine now

@jroweboy jroweboy force-pushed the jroweboy:qtifw_build_installer branch 3 times, most recently from dc8adb1 to 9c7d926 Sep 30, 2017

Installer: Address review comments
Correctly set permissions on mac installer and create a missing folder

@jroweboy jroweboy force-pushed the jroweboy:qtifw_build_installer branch from 9c7d926 to e7da39a Sep 30, 2017

@j-selby
Copy link
Contributor

j-selby left a comment

citra-emu/ext-windows-bin#11 fixes a number of bugs with this.

<Publisher>Citra team</Publisher>
<!-- e.g. /home/<user>/Citra -->
<TargetDir>@HomeDir@/Citra</TargetDir>
<InstallerApplicationIcon>icon</InstallerApplicationIcon>

This comment has been minimized.

@j-selby

j-selby Oct 7, 2017

Contributor
<TargetDir>@HomeDir@/Citra</TargetDir>
<AdminTargetDir>/opt/citra</AdminTargetDir>

maybe?

<Publisher>Citra team</Publisher>
<StartMenuDir>Citra</StartMenuDir>
<!-- e.g. C:\home\<user>\AppData\Local\Citra -->
<TargetDir>@HomeDir@/AppData/Local/Citra</TargetDir>

This comment has been minimized.

@j-selby

j-selby Oct 7, 2017

Contributor

Do we want this in %appdata% or C:\Program Files, now we have a mechanism to write there (with QtIFW 3)?

@j-selby j-selby referenced this pull request Oct 8, 2017

Closed

Updater QT Integration #2997

<Name>Citra</Name>
<Version>1.0.0</Version>
<Title>Citra Updater</Title>
<Publisher>Citra team</Publisher>

This comment has been minimized.

@chris062689

chris062689 Oct 11, 2017

Member

Didn't we decide to change this?

This comment has been minimized.

@jroweboy

jroweboy Oct 11, 2017

Member

check the latest commit

@j-selby
Copy link
Contributor

j-selby left a comment

Change /opt/Citra to /opt/citra and LGTM!

@jroweboy jroweboy force-pushed the jroweboy:qtifw_build_installer branch from 6971120 to 1fc1e2b Oct 12, 2017

@jroweboy jroweboy merged commit e1bb198 into citra-emu:master Oct 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jroweboy jroweboy referenced this pull request Feb 3, 2018

Merged

Progress Report 2017 Fourth Quarter #73

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment