-
Notifications
You must be signed in to change notification settings - Fork 79
delete leftover directories #169
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
base: master
Are you sure you want to change the base?
Conversation
When stripping XCTests, some `.framework` folders will left empty. Such folders are expected to have a valid framework inside and installing the resultting `.ipa` will result in errors. Make sure that any directory that was empties as a result of our stripping process is also deleted.
| removedDirs = new Set<string>(); | ||
| for (const d of dirsToCheck) { | ||
| if (fs.readdirSync(d).length === 0) { | ||
| this.emit('message', 'Deleting ' + d); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use “ instead of ‘ (see ci complains)
| walk.walkSync(dir, (basedir: string, filename: string, stat: any) => { | ||
| const target = path.join(basedir, filename); | ||
| // if (target.toLowerCase().indexOf('/xct') !== -1) | ||
| if (target.toLowerCase().indexOf("xctest") !== -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (target.toLowerCase().indexOf("xctest") !== -1) { | |
| if (target.toLowerCase().indexOf(".xctest") !== -1) { |
This check is probably safer
| dirsToCheck.add(basedir); | ||
| } | ||
| }); | ||
| let removedDirs: Set<string> = new Set<string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s unclear to me the need to define a set of paths, an array should be fine and simpler because you cant have two different paths with the same path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplify the loop iteration, the filter to remove the element from the set is adding an extra complication that is not necessary.
| if (fs.readdirSync(d).length === 0) { | ||
| this.emit('message', 'Deleting ' + d); | ||
| fs.rmdirSync(d); | ||
| removedDirs.add(d); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what i understand this code is deleting empty directories. But if an empty directory is contained within a directory that only had this one it wont be removed, so the removal must happen recursively.
When stripping XCTests, some
.frameworkfolders will left empty. Such folders are expected to have a valid framework inside and installing the resultting.ipawill result in errors.Make sure that any directory that was empties as a result of our stripping process is also deleted.