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.

Sign up for GitHub

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

Jump to bottom

Ensure generate works on additional disk volumes #327

Merged
merged 2 commits into from
Apr 18, 2019
Merged

Ensure generate works on additional disk volumes #327

merged 2 commits into from
Apr 18, 2019

Conversation

kwridan
Copy link
Collaborator

@kwridan kwridan commented Apr 17, 2019 •
edited
Loading

Resolves #324

Short description 📝

Running tuist generate on external volumes fails:

e.g.

$ cd /Volumes/RAMDisk/Project
$ tuist generate
Generating workspace Workspace.xcworkspace
Generating project Framework2
❌ Error: The file “Framework2.xcodeproj” couldn’t be saved in the folder “Framework2”.

Solution 📦

Using replaceItemAt has a caveat where it requires the item and its replacement to be on the same volume (see replaceItemAt documentation)

To mitigate this, the documentation suggests creating a temporary file within that volume where the new item can be moved to first before performing the replace.

Implementation 👩‍💻👨‍💻

  • Update FileHandler.replace
  • Update changelog

Test Plan 🛠

  • Manually create a RAM disk e.g.
diskutil erasevolume HFS+ “RAMDisk” `hdiutil attach -nomount ram://524288`

(this creates a 250MB RAM disk)

  • Create a new folder within that disk
  • run tuist init
  • run tuist generate
  • Verify the generation succeeds

@kwridan
Ensuring generate works on additional disk volumes
041468a 
Resolves tuist#324

- Using `replaceItemAt` has a caveat where it requires the item and its replacement to be on the same volume (see https://developer.apple.com/documentation/foundation/filemanager/2293212-replaceitemat)
- To mitigate this, the documentation suggests creating a temporary file within that volume
- The item can be moved there first
- Then a `replaceItemAt` can be performed

Test Plan:

- Manually create a RAM disk e.g.  `diskutil erasevolume HFS+ “RAMDisk” `hdiutil attach -nomount ram://524288`` (this creates a 250MB RAM disk)
- Create a new folder within that disk
- run `tuist --init`
- run `tuist generate`
- Verify the generation succeeds
@tuistbot
Copy link
Contributor

tuistbot commented Apr 17, 2019 •
edited
Loading

1 Warning
⚠️ Have you introduced any user-facing changes? If so, please take some time to update the documentation. Keeping the documentation up to date makes it easier for users to learn how to use Tuist.

Generated by 🚫 Danger

@kwridan kwridan changed the title Ensuring generate works on additional disk volumes Ensure generate works on additional disk volumes Apr 17, 2019
kwridan
kwridan commented Apr 17, 2019
View reviewed changes
Sources/TuistGenerator/Generator/GeneratedProject.swift
@@ -37,9 +37,7 @@ final class GeneratedProject {
/// - Parameter path: Path to the project (.xcodeproj)
/// - Returns: GeneratedProject instance.
func at(path: AbsolutePath) throws -> GeneratedProject {
let xcode = try XcodeProj(pathString: path.pathString)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initially before testing on a RAM Disk, I was testing on Google Drive :p - and there I found this step fails sometimes as the generated file fails to be read from disk (if done immediately).

@kwridan
Copy link
Collaborator Author

kwridan commented Apr 17, 2019 •
edited
Loading

Technically we can automate a test for this using the steps described above, though I worry this would be flakey as it involves mounting / un-mounting disks :/ - as such may not be worthwhile.

@codecov
Copy link

codecov bot commented Apr 17, 2019 •
edited
Loading

Codecov Report

Merging #327 into master will increase coverage by 0.01%.
The diff coverage is 96.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
+ Coverage   89.47%   89.48%   +0.01%     
==========================================
  Files         278      278              
  Lines       12354    12366      +12     
==========================================
+ Hits        11054    11066      +12     
  Misses       1300     1300
Impacted Files Coverage Δ
...neratorTests/Generator/ProjectGeneratorTests.swift 98.57% <100%> (ø) ⬆️
...es/TuistGenerator/Generator/GeneratedProject.swift 100% <100%> (ø) ⬆️
Sources/TuistCore/Utils/FileHandler.swift 80.7% <96.29%> (+6.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96424de...a285870. Read the comment docs.

@pepicrft
Copy link
Contributor

Technically we can automate a test for this using the steps described above, though I worry this would be flakey as it involves mounting / un-mounting disks :/ - as such may not be worthwhile.

Agree. Having addressed this scenario and considering that we don't touch that logic much I think the trade-off of not having tests vs having flakey tests is worth it.

@kwridan kwridan merged commit 481e8be into tuist:master Apr 18, 2019
@kwridan kwridan deleted the fixes/324-replace-item-at-volume branch April 18, 2019 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepicrft pepicrft pepicrft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

❌ Error: The file couldn’t be saved in the folder
3 participants
@kwridan @tuistbot @pepicrft