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

Software licensing, support for pre-10.11 #47

Merged
merged 17 commits into from Feb 19, 2020
Merged

Software licensing, support for pre-10.11 #47

merged 17 commits into from Feb 19, 2020

Conversation

dxdc
Copy link
Contributor

@dxdc dxdc commented Jan 28, 2020

  • Ability to add software licensing via one of the following files (in the same folder as the app): license.txt, license.rtf, sla.r. If none of these files are present, nothing will be added.

Fixes #44

  • Modify to use full system paths

  • Support for pre-10.11 images (detected encoding based on minimum runtime for app)

@sindresorhus
Copy link
Owner

Thanks for contributing 🙌

Ability to add software licensing via one of the following files (in the same folder as the app): license.txt, license.rtf, sla.r. If none of these files are present, nothing will be added.

I think it would be better to add this to https://github.com/LinusU/node-appdmg, which is the package handling the actual DMG creation.

Potential fix for #44

👍

Modify to use full system paths

👍

Support for pre-10.11 images (detected encoding based on minimum runtime for app)

Sorry, not interested in this.

@dxdc
Copy link
Contributor Author

dxdc commented Jan 28, 2020

I think it would be better to add this to https://github.com/LinusU/node-appdmg, which is the package handling the actual DMG creation.

Would take some refactoring to generalize it. Rather than build a specific one for every language, I tried to just keep it simple. appdmg seems to support a lot more options.

Support for pre-10.11 images (detected encoding based on minimum runtime for app)

Why? It's just a graceful fallback and won't impact things either way. Otherwise, it will fail on pre-10.11.

@dxdc
Copy link
Contributor Author

dxdc commented Jan 28, 2020

All tests passing @sindresorhus

@sindresorhus
Copy link
Owner

This part could use some code comments. It's not very easy to follow: https://github.com/sindresorhus/create-dmg/pull/47/files#diff-2cce40143051e25f811b56c79d619bf5R150-R192

cli.js Outdated
@@ -91,6 +98,12 @@ async function init() {
ora.text = 'Creating icon';
const composedIconPath = await composeIcon(path.join(appPath, 'Contents/Resources', `${appIconName}.icns`));

ora.text = 'Checking minimum runtime';
const {stdout: minSystemVersion} = await execa('/usr/libexec/PlistBuddy', ['-c', 'Print :LSMinimumSystemVersion', infoPlistPath]);
Copy link
Owner

Choose a reason for hiding this comment

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

We already have the plist loaded as the appInfo variable. You can use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. Fixed.

cli.js Outdated
// Adapted from:
// https://github.com/electron-userland/electron-builder/tree/master/packages/dmg-builder/src

function getRtfUnicodeEscapedString(text) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this and the below methods should be moved to a util.js file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved all logic to sla.js file

cli.js Outdated
if (hasRaw || hasRtf || hasTxt) {
ora.text = 'Adding Software License Agreement';

const tempDmgPath = tempy.file({extension: 'dmg'});
Copy link
Owner

Choose a reason for hiding this comment

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

The logic from here and until line 191 should be moved out into a separate function, including the above variables. It could be named addLicenseAgreementIfNeeded() or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up moving the entire SLA logic to a separate file

cli.js Outdated
data += '\ndata \'RTF \' (5000, "English") {\n';

if (hasRtf) {
data += serializeString((Buffer.from(fs.readFileSync(rtfSlaFile, 'utf8')).toString('hex').toUpperCase()));
Copy link
Owner

Choose a reason for hiding this comment

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

Buffer.from(fs.readFileSync(rtfSlaFile, 'utf8'))

If you drop the utf8 the file will be read directly as a buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

readme.md Outdated

It will try to code sign the DMG, but the DMG is still created and fine even if the code signing fails, for example if you don't have a developer certificate.

<img src="screenshot-dmg.png" width="772">

### Software license

If `license.txt`, `license.rtf`, or `sla.r` (raw SLAResources file) are present in the same folder as the app, they will be added as a software agreement when opening the image. The image will not be mounted unless the user indicates agreement with the license.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you linkify raw SLAResources file to a source where the user can read more about raw SLAResource files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

readme.md Outdated

If `license.txt`, `license.rtf`, or `sla.r` (raw SLAResources file) are present in the same folder as the app, they will be added as a software agreement when opening the image. The image will not be mounted unless the user indicates agreement with the license.

`/usr/bin/rez` (Xcode Developer Tools) must be installed.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
`/usr/bin/rez` (Xcode Developer Tools) must be installed.
`/usr/bin/rez` (Xcode Command Line Tools) must be installed.

And linkify it to https://developer.apple.com/download/more/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cli.js Outdated
@@ -91,6 +98,12 @@ async function init() {
ora.text = 'Creating icon';
const composedIconPath = await composeIcon(path.join(appPath, 'Contents/Resources', `${appIconName}.icns`));

ora.text = 'Checking minimum runtime';
const {stdout: minSystemVersion} = await execa('/usr/libexec/PlistBuddy', ['-c', 'Print :LSMinimumSystemVersion', infoPlistPath]);
const minorVersion = Number(minSystemVersion.replace('10.', '')) || 0;
Copy link
Owner

Choose a reason for hiding this comment

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

This check is not good enough:

'10.10.2'.replace('10.', '')
//=> "10.2"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Added some modified logic, hope you agree. It's not as sophisticated as a true 'version compare' but hopefully will be sufficient. Otherwise, we can replace this with a true version compare logic (which can compare 10.10.1a for example).

Choose a reason for hiding this comment

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

On our usage we've noticed one machine was making bad DMGs.
I'm now investigating the Xcode diffs. but this logic will fail if no LSMinimumSystemVersion exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@talaviram normally this is included by default in XCode. Are you running into a situation where it's not?

Something like:

otool -l /usr/local/go/bin/go | grep -B1 -A3 MIN_MACOS

Can also be used to define min versioning, but it will need some more work to incorporate.

cli.js Outdated
@@ -65,6 +66,12 @@ try {
throw error;
}

let dmgFormat;
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't need to be top-level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

cli.js Outdated
appInfo = plist.parse(stdout);
}

const appName = appInfo.CFBundleDisplayName || appInfo.CFBundleName;
const appIconName = appInfo.CFBundleIconFile.replace(/\.icns/, '');
const dmgTitle = appName.length > 27 ? (cli.flags['dmg-title'] || appName) : appName;
const dmgTitle = appName.length > 27 ? (cli.flags['dmgTitle'] || appName) : appName;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const dmgTitle = appName.length > 27 ? (cli.flags['dmgTitle'] || appName) : appName;
const dmgTitle = appName.length > 27 ? (cli.flags.dmgTitle || appName) : appName;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

cli.js Outdated
@@ -91,6 +98,12 @@ async function init() {
ora.text = 'Creating icon';
const composedIconPath = await composeIcon(path.join(appPath, 'Contents/Resources', `${appIconName}.icns`));

ora.text = 'Checking minimum runtime';
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this needs a step. We'll just show the outcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@dxdc
Copy link
Contributor Author

dxdc commented Feb 10, 2020

@sindresorhus Addressed all code review changes

@sindresorhus sindresorhus changed the title Software licensing, support for pre-10.11, #44 Software licensing, support for pre-10.11 Feb 19, 2020
@sindresorhus sindresorhus merged commit 7d684a2 into sindresorhus:master Feb 19, 2020
@sindresorhus
Copy link
Owner

Looks good now. Thanks! :)

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.

More verbose errors would be useful
3 participants