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

Linux build #161

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Linux build #161

wants to merge 7 commits into from

Conversation

smythe811
Copy link

Modifications to allow for a clean build on the Linux OS, and sample template for installation on a shared web hosting provider.

build.xml Outdated
@@ -26,6 +26,9 @@
<condition property="is_windows">
<os family="windows"/>
</condition>
<condition property="is_linux">
<os family="unix"/>
Copy link
Owner

Choose a reason for hiding this comment

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

The unix family apparently includes MacOS, so is_linux (and the soffice value) needs to be calculated as something like this:

<and><os family="unix"/><not><os family="mac"/></not></and>

Copy link
Author

Choose a reason for hiding this comment

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

Noted, I do not have a mac to test this on. I will make the indicated change if someone can test this.

Copy link
Owner

Choose a reason for hiding this comment

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

I can confirm that this expression works to exclude macOS.

@@ -0,0 +1,7 @@
<?php
// Use this template in your ./local/ difrectory to set the default location of
Copy link
Owner

Choose a reason for hiding this comment

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

directory

@@ -19,6 +19,6 @@ public static Profile profile() {
SerialPort.PARITY_NONE)
.max_lanes(6)
.setup("RF")
.match(" *([A-Z])=(\\d\\.\\d+)([^ ]?)", Event.LANE_RESULT, 1, 2));
.match(" *([A-Z])=(\\d\\.\\d+)([^ ]?)", Event.LANE_RESULT, 1, 2);
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, that's embarrassing. Thank you.

docs/build.xml Outdated
<!-- Set soffice for Windows if that's where we are, otherwise use
standard (Mac) value -->
<!-- Set soffice for Windows if that's where we are, or Linux if we are There
otherwise use standard (Mac) value -->
Copy link
Owner

Choose a reason for hiding this comment

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

Please lowercase "there" and fix spacing between "use" and "standard"

</condition>
<condition property="soffice"
value="/usr/bin/soffice">
<os family="unix"/>
Copy link
Owner

Choose a reason for hiding this comment

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

As with is_linux property, this needs a more nuanced condition.

@@ -357,7 +360,7 @@
</exec>
</target>

<target name="dist.oneclick" depends="generated, timer-jar, docs-dist"
<target name="dist.oneclick" if="is_windows" depends="generated, timer-jar, docs-dist"
Copy link
Owner

Choose a reason for hiding this comment

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

This target actually works on non-Windows platforms (as long as the Uniserver files are alongside).

Copy link
Author

Choose a reason for hiding this comment

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

These would need to be manually installed alongside as line 327 does not extract them on non-windows os.
<target name="unpack-uniserver" if="is_windows"

Is there a reason to build the Windows installer on Linux?

Copy link
Owner

Choose a reason for hiding this comment

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

I normally build all the distributable artifacts on MacOS. You're correct that extracting the UniServer files has to be done on Windows, but then the files can be copied elsewhere.

Rather than conditionalize the target itself, I'd rather conditionalize the behavior to test for the presence of the UniServer sources, and present an obvious but non-fatal message if they're absent.

http://ant.apache.org/manual/Tasks/available.html

docs/build.xml Outdated
</condition>
<property name="soffice"
location="/Applications/LibreOffice.app/Contents/MacOS/soffice"/>
<condition property="is_windows">
<os family="windows"/>
</condition>
<condition property="is_linux">
<os family="unix"/>
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment as elsewhere.

@@ -0,0 +1,7 @@
<?php
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not seeing where this is used in the build process, or how this fits in generally.

Copy link
Author

Choose a reason for hiding this comment

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

This is a template file for manual install, it is possible there is a better location for it. This file appears to be built by the installer, but when deploying the website without the installer, as in the case of a manually deployed shared server this file is missing, but required to provide a working path to the users home directory for sqlite files when a user can not write to the main /var location.

Copy link
Owner

Choose a reason for hiding this comment

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

Would your purpose be better served by adding a new target, not included in the default 'dist' target, that performs a manual install from source? You could specify the default file path on the ant command line as a property (-D or -propertyfile) along with the destination directory. The build script could then instantiate the template in place, in the manner of banner.inc and generated-version.inc.

That would also address the dist.oneclick issue, as you would only need to build the linux-in-situ target, rather than the default target that tries to build all platforms' artifacts.

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.

2 participants