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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache the helpers module #691

Merged
merged 3 commits into from
Nov 19, 2019
Merged

Cache the helpers module #691

merged 3 commits into from
Nov 19, 2019

Conversation

pepicrft
Copy link
Contributor

Related #668

Short description 馃摑

This PR adds support for caching the project description helpers modules across runs. If any of the files under the helpers directory changes, the logic deletes the old version of the framework automatically.

Solution 馃摝

Extend the ProjectDescriptionHelpersBuilder logic to cache the frameworks in a system directory.

@pepicrft pepicrft self-assigned this Nov 18, 2019
@pepicrft pepicrft requested a review from a team November 18, 2019 16:19
@ghost ghost requested review from kwridan and ollieatkinson and removed request for a team November 18, 2019 16:19
@pepicrft pepicrft mentioned this pull request Nov 18, 2019
11 tasks
Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Nice work - looks good overall

let dylibName = "libProjectDescriptionHelpers.dylib"

if FileHandler.shared.exists(helpersModuleCachePath) {
return helpersModuleCachePath.appending(component: dylibName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a further optimization, you could cache the known built dylib paths in an in memory cache (saves calculating hashes repeatedly for every manifest)

let helpersPath = path.appending(RelativePath("Tuist/ProjectDescriptionHelpers"))
try FileHandler.shared.createFolder(helpersPath)
try FileHandler.shared.write("import Foundation; class Test {}", path: helpersPath.appending(component: "Helper.swift"), atomically: true)
let projectDescriptionPath = try resourceLocator.projectDescription()

// When
_ = try subject.build(at: path, projectDescriptionPath: projectDescriptionPath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the purpose of adding two calls here to test out caching?

If so, perhaps we could do that in a separate test that tests for it a bit more explicitly?

e.g.

// When
let paths = try (0..<3).map { _ in try subject.build(at: path, projectDescriptionPath: projectDescriptionPath) }

// Then
// check all the paths are equal 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. I changed the current test to follow your approach instead.

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.

None yet

2 participants