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

Create target upfront #112

Merged
merged 6 commits into from
Oct 23, 2017
Merged

Create target upfront #112

merged 6 commits into from
Oct 23, 2017

Conversation

cstruct
Copy link
Collaborator

@cstruct cstruct commented May 22, 2016

Open target with write and exclusive flag.
This closes #65

lib/appdmg.js Outdated
} else {
next(null)
}
})

fs.open(global.target, 'wx', function (error) {
Copy link
Owner

Choose a reason for hiding this comment

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

fs.writeFile with empty string, as it is now the file descriptor is kept open

@albinekb
Copy link

ping @cstruct 🦄

@cstruct cstruct force-pushed the fix/precreate-target branch from a053863 to 0755d48 Compare October 22, 2017 10:31
@cstruct
Copy link
Collaborator Author

cstruct commented Oct 22, 2017

Rebased on master and issues have been fixed.

@albinekb
Copy link

Good job @cstruct 🙌

@LinusU
Copy link
Owner

LinusU commented Oct 22, 2017

That last commit, won’t that open us up to leaving empty files behind?

@cstruct
Copy link
Collaborator Author

cstruct commented Oct 23, 2017

Yes it will... Adding the cleanup should be moved to the callback of the Looking for target step since it's the first place we know we won't delete a preexisting file. I'll fix it this afternoon.

Copy link

@albinekb albinekb left a comment

Choose a reason for hiding this comment

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

RE-LGTM

lib/appdmg.js Outdated
@@ -80,6 +80,14 @@ module.exports = exports = function (options) {
fs.writeFile(global.target, '', { flag: 'wx' }, function (err) {
if (err && err.code === 'EEXIST') return next(new Error('Target already exists'))

pipeline.addCleanupStep('unlink-target', 'Removing target image', function (next, hasErrored) {
Copy link
Owner

Choose a reason for hiding this comment

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

err could potentially be non-null here in which case the file won't exists. Maybe add if (err) return next(err) before?

Copy link
Owner

Choose a reason for hiding this comment

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

actually, it might be that if fs.writeFile returns an err, we do not know if the file is there or not. In that case we should always unlink it, but ignore any ENOENT errors...

Not super important to get this done now though, if you're short on time 👍

Copy link
Owner

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Great!

@cstruct cstruct merged commit deb0ee7 into master Oct 23, 2017
@cstruct cstruct deleted the fix/precreate-target branch October 23, 2017 20:55
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.

Better error message when target directory isn't present
3 participants