Skip to content

Conversation

mabaasit
Copy link
Collaborator

Description

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

debug('macnotary result:', macnotaryResult.stdout, macnotaryResult.stderr);
debug('ls', (await execFile('ls', ['-lh'], { cwd: path.dirname(src), encoding: 'utf8' })).stdout);

// Step:3 - clean up. remove existing src, unzip signedArchive to src, remove signedArchive and unsignedArchive
Copy link
Contributor

Choose a reason for hiding this comment

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

q: do you think we need to be concerned about clean up resources (step 3 here) if step 1/step 2 fails? as it's written, if anything throws (including any step in the cleanup), we won't clean up any temporary files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if for some reason any step here fails, the whole build process will fail.

Copy link
Collaborator

@mcasimir mcasimir Jan 18, 2024

Choose a reason for hiding this comment

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

Not a big deal, but since is not a big effort, it may be nice to move the removal calls in a finally block.

// eslint-disable-next-line strict
'use strict';
const chalk = require('chalk');
const childProcess = require('child_process');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice seeing some line of code leaving this file! :)

@mabaasit mabaasit merged commit 26ace65 into main Jan 18, 2024
@mabaasit mabaasit deleted the notarize-dmg branch January 18, 2024 16:30
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.

4 participants