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

Removes Lintian warning and errors #96

Merged
merged 7 commits into from
Sep 27, 2017

Conversation

fcastilloec
Copy link
Collaborator

The warnings non-standard-dir-perm, non-standard-file-perm, extra-license-file are addressed by this PR.
As we stand, only debian-changelog-file-missing and binary-without-manpage are the only lintianOverrides needed. Any other warning or errors are related to the app being packaged and not electron-installer-debian
The test provided uses app-with-asar so there's only one file permission to worried. All files and directories permissions inside app-with-asar have been change from 0664 -> 0644 and 0775 -> 0755. This change is in accordance with debian rules and will pass all lintian checks address by this PR.

This will remove 'non-standard-dir-perm', 'non-standard-file-perm' and
'extra-license-file' from lintian
Permissions have been changed to all files inside "app with asar" in
order to supress all lintian errors and warnings. There are still
lintianOverrides but none related to file permissions
The deb packages where being deleted by mocha at the end of each test so
lintian was not outputing anything. There's a test specific to lintian
now
@fcastilloec
Copy link
Collaborator Author

I have rebased my PR with all the latest commits. @malept or @unindented could you briefly check the code and either approve or request changes before I merge this request?

.travis.yml Outdated
@@ -11,6 +11,7 @@ addons:
packages:
- fakeroot
- lintian
- fakeroot
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is duplicate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, the result of rebasing and eslint doesn't check for this

@@ -11,6 +11,7 @@ var glob = require('glob')
var path = require('path')
var temp = require('temp').track()
var wrap = require('word-wrap')
var mkdirp = require('mkdirp')
Copy link
Collaborator

Choose a reason for hiding this comment

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

fs-extra already has this functionality (https://github.com/jprichardson/node-fs-extra/blob/master/docs/ensureDir.md), so no need to add more dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Appears to be unofficially supported: jprichardson/node-fs-extra#60

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess when it becomes officially supported we can switch to using only fs-extra in the meantime we should stick with mkdirp

async.apply(fs.symlink, binSrc, binDest, 'file')
], function (err) {
callback(err && new Error('Error creating binary file: ' + (err.message || err)))
mkdirp(binDir, '0755', function (err, made) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, but fs-extra doesn't have chmod options. Oh well, another dependency it is then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we're going to need a new dependency until fs-extra decides to include chmod options to their commands

@fcastilloec fcastilloec merged commit 9135915 into electron-userland:master Sep 27, 2017
@fcastilloec fcastilloec deleted the lintian branch September 28, 2017 19:16
malept added a commit that referenced this pull request Oct 4, 2018
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.

3 participants