Add fetch-interval to `outdated` command to skip fetching the dependencies #2714

mollyIV · 2019-02-24T19:21:54Z

Motivation and Context

carthage outdated --xcode-warnings (Xcode build phase) command fetches the dependencies each time when executed. In order to improve the performance of the command, we'd like to introduce a new parameter fetch-interval. This parameter specifies how often the dependencies should be fetched from the remote location. Example usage:

carthage outdated --xcode-warning --fetch-interval 30

fetch-interval - the minimum amount of time that must elapse between dependencies fetch operation (provided in ~~minutes~~ seconds)

#2698

Description

~~This PR is draft one [WIP]. Kindly ask you to check the review comments~~ 🙏
EDIT: PR is ready for review 🙏

It's my first Carthage contribution 🎉

Source/carthage/Outdated.swift

mollyIV · 2019-02-24T19:31:03Z

Source/carthage/Outdated.swift

+                if shouldIgnoreFetching {
+                    // TODO: Non-fetching changes required.
+                    carthage.println("The dependencies will not be fetched due to non-expired fetch interval.")
+                    return project.outdatedDependencies(options.isVerbose)


I noticed that we do download the remote dependencies when we initialise ResolverProtocol type object:

Carthage/Source/CarthageKit/Project.swift

Lines 505 to 509 in 1c3e23a

let resolver = resolver ?? resolverType.init(

versionsForDependency: versions(for:),

dependenciesForDependency: dependencies,

resolvedGitReference: resolvedGitReference

)

Wondering if modifying the resolver initialisation is a way to go, i.e. passing [:] to resolver's versions 🤔

Could you guys please give me a hint what would be the best way to ignore fetching the remote dependencies? 🙏

I think you can just set FetchCache.fetchCacheInterval to the value that's passed in. That should be all you need to do?

👋 @mdiep Thanks for the comment and your help 🙇 Just to double check: FetchCache.fetchCacheInterval is just a static property without the persistent storage behind it, so we need the UserDefaults logic I introduced in this PR anyway, right?

With the suggested changes we are going to have:

... if shouldIgnoreFetching { carthage.println("The dependencies will not be fetched due to non-expired fetch interval.") FetchCache.fetchCacheInterval = TimeInterval(options.fetchInterval) return project.outdatedDependencies(options.isVerbose) } ....

Could you please just confirm we are on the same page? 🙏

so we need the UserDefaults logic I introduced in this PR anyway, right?

Why?

Please correct me if I am wrong @mdiep , but I though carthage (CLI application) process is killed as soon as the command execution ends. It means carthage does not hold in memory state (the static properties) between the command executions. That's why I introduced UserDefaults - to keep the state of the application between the command executions.

I prefer option one.

But failing that, I think we should keep the last fetched time in a file on disk in the dependencies directory. Users sometimes delete the entire cache to get back to a clean state. I'd prefer to keep all the cache info in that directory instead of putting some of it in user defaults.

👋 @mdiep

I was investigating option one and unfortunately couldn't find a reasonable solution.

I was playing with with the option number two - file solution. I'd suggest to modify FetchCache component and convert it from in-process caching to persistent caching. We could store last fetch timestamp per dependency (../Library/Caches/org.carthage.CarthageKit/dependencies):

Please tell me what you think about this solution 🙇

What if we set the last modified date of the directory for the dependency?

Good idea, thanks! 👍

Tested on just three dependencies, but already noticed a great improvement (a few seconds):

I implemented the changes and kindly ask you for re-review @mdiep 🙇 🙇

👋 Hey @mdiep 🙂 Just wanted to kindly ask if you did get a chance to have a look at the newest changes? 🙇 🤔

Source/carthage/Outdated.swift

mdiep · 2019-03-04T13:51:01Z

Source/carthage/Outdated.swift

+                if shouldIgnoreFetching {
+                    // TODO: Non-fetching changes required.
+                    carthage.println("The dependencies will not be fetched due to non-expired fetch interval.")
+                    return project.outdatedDependencies(options.isVerbose)


I think you can just set FetchCache.fetchCacheInterval to the value that's passed in. That should be all you need to do?

mollyIV · 2019-04-17T05:32:36Z

👋 @jdhealy

Thanks for adding yourself as a reviewer 🙇 Please let me know if you have any questions about the implementation 👍

jdhealy · 2019-04-24T12:18:12Z

It's my first Carthage contribution 🎉

High five over the internet Thanks, so much!

I've got time for one or two notes before I head out the door this morning.

jdhealy · 2019-04-24T12:18:26Z

Source/CarthageKit/Git.swift

+    fileprivate static func repositoriesLastFetchTime() -> Date? {
+        do {
+            let attr = try FileManager.default.attributesOfItem(atPath: Constants.Dependency.repositoriesURL.path)
+            return attr[FileAttributeKey.modificationDate] as? Date


For us relying on file system modification date here — there's far too many antivirus scanners and cloud storage sync tools that continually just modify large swaths of files for me to be comfortable with this 😕

Maybe our own xattr (extended attribute), could work? I'll try to give some thought to this later…

Thank you for your suggestion @jdhealy 🙇 I updated the code - now we are using custom extended attribute instead of modification date attribute 💪

Could you please have a 👀 🙏

EDIT: Noticed CI job has failed, but it seems like a global issue. It happened for other pull requests as well 🤔

tmspzz · 2019-05-20T15:28:57Z

@mollyIV It doesn't look like this PR is building. Do you have time to follow up?

mollyIV · 2019-05-20T15:53:41Z

👋 @blender ,
It might be a global issue #2780 - I saw many PRs with failing tests.

Will do the master rebase 👍

mollyIV · 2019-05-21T05:18:36Z

Hey @blender,
I did the rebase however the tests are still failing. I compered the CI_INTEGRATION_TESTS from this MR

and the error log seems to be the same.

Any hint what that could relate to?
Shall we create a new issue about fixing those? (seems a global issue)

🤔

stale · 2019-06-20T05:38:49Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mollyIV · 2019-06-20T06:10:56Z

Hey guys 👋

Any news about a CI issue? 🤔 Could anyone try to re-run it? 🙏

…ncies

… of a directory

stale · 2019-08-28T18:39:50Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mollyIV · 2019-09-02T06:12:45Z

👋@Stale, still in review.

stale · 2019-10-02T06:57:16Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mollyIV · 2019-10-02T07:56:58Z

ping stale 🤖

stale · 2019-11-01T08:12:05Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mollyIV · 2019-11-01T12:52:31Z

ping stale

stale · 2019-12-01T13:45:21Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

DavidBrunow · 2019-12-08T21:38:20Z

Source/CarthageKit/Git.swift

@@ -22,13 +22,12 @@ public struct FetchCache {
 		lastFetchTimes.removeAll()
 	}

-	internal static func needsFetch(forURL url: GitURL) -> Bool {
-		guard let lastFetch = lastFetchTimes[url] else {
+    internal static func needsFetch(forURL url: GitURL) -> Bool {


Carthage uses tabs instead of spaces for indentation -- could you please reformat your changes to do the same?

DavidBrunow · 2019-12-08T21:38:34Z

Source/CarthageKit/Git.swift

 			return true
 		}

 		let difference = Date().timeIntervalSince1970 - lastFetch
-


Could you please revert this change?

DavidBrunow · 2019-12-08T21:56:19Z

Source/CarthageKit/Git.swift

+    }
+
+    func extendedAttribute(for name: String) throws -> Data  {
+        let data = try withUnsafeFileSystemRepresentation { path -> Data in


Curious -- what are your thoughts on using this approach rather than using the methods provided by NSFileManager? I've never used either personally.

https://developer.apple.com/documentation/foundation/filemanager/1413667-setattributes

https://developer.apple.com/documentation/foundation/filemanager/1410452-attributesofitem

mollyIV commented Feb 24, 2019

View reviewed changes

Source/carthage/Outdated.swift Outdated Show resolved Hide resolved

mollyIV commented Feb 24, 2019

View reviewed changes

Source/carthage/Outdated.swift Outdated Show resolved Hide resolved

mollyIV commented Feb 24, 2019

View reviewed changes

mollyIV mentioned this pull request Feb 24, 2019

Speeding up carthage outdated build phases #2698

Open

mdiep requested changes Mar 4, 2019

View reviewed changes

mollyIV changed the title ~~[WIP] Add fetch-interval to outdated command to skip fetching the dependencies~~ Add fetch-interval to outdated command to skip fetching the dependencies Mar 7, 2019

mollyIV force-pushed the carthage-outdated-skip-fetching-dependencies branch from 86a4d9d to 16a2b51 Compare April 1, 2019 06:25

mollyIV marked this pull request as ready for review April 1, 2019 06:28

jdhealy self-requested a review April 1, 2019 06:36

jdhealy reviewed Apr 24, 2019

View reviewed changes

mollyIV force-pushed the carthage-outdated-skip-fetching-dependencies branch from fc5440d to bfca280 Compare May 20, 2019 15:55

stale bot added the stale label Jun 20, 2019

stale bot removed the stale label Jun 20, 2019

This comment has been minimized.

Sign in to view

stale bot added the stale label Jul 20, 2019

This comment has been minimized.

Sign in to view

stale bot removed the stale label Jul 22, 2019

mollyIV added 3 commits July 29, 2019 20:02

Add fetch-interval to outdated command to skip fetching the depende…

63777fa

…ncies

Add persistent cache to FetchCache component

e029df5

Use custom extended attributes instead of modification date attribute…

3796768

… of a directory

mollyIV force-pushed the carthage-outdated-skip-fetching-dependencies branch from bfca280 to 3796768 Compare July 29, 2019 18:02

stale bot added the stale label Aug 28, 2019

stale bot removed the stale label Sep 2, 2019

stale bot added the stale label Oct 2, 2019

stale bot removed the stale label Oct 2, 2019

tmspzz mentioned this pull request Oct 10, 2019

Roadmap Q4 2019 - Q1 2020 #2890

Closed

stale bot added the stale label Nov 1, 2019

stale bot removed the stale label Nov 1, 2019

stale bot added the stale label Dec 1, 2019

stale bot closed this Dec 8, 2019

DavidBrunow reviewed Dec 8, 2019

View reviewed changes

tmspzz added the enhancement label Dec 8, 2019

tmspzz reopened this Dec 8, 2019

stale bot removed the stale label Dec 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fetch-interval to `outdated` command to skip fetching the dependencies #2714

Add fetch-interval to `outdated` command to skip fetching the dependencies #2714

mollyIV commented Feb 24, 2019 •

edited

mollyIV Feb 24, 2019

mdiep Mar 4, 2019

mollyIV Mar 4, 2019

mdiep Mar 4, 2019

mollyIV Mar 4, 2019

mdiep Mar 20, 2019

mollyIV Mar 31, 2019

mdiep Mar 31, 2019

mollyIV Apr 1, 2019

mollyIV Apr 24, 2019

mdiep Mar 4, 2019

mollyIV commented Apr 17, 2019

jdhealy commented Apr 24, 2019

jdhealy Apr 24, 2019

mollyIV Apr 27, 2019 •

edited

tmspzz commented May 20, 2019

mollyIV commented May 20, 2019

mollyIV commented May 21, 2019

stale bot commented Jun 20, 2019

mollyIV commented Jun 20, 2019

This comment has been minimized.

This comment has been minimized.

stale bot commented Aug 28, 2019

mollyIV commented Sep 2, 2019

stale bot commented Oct 2, 2019

mollyIV commented Oct 2, 2019

stale bot commented Nov 1, 2019

mollyIV commented Nov 1, 2019

stale bot commented Dec 1, 2019

DavidBrunow Dec 8, 2019

DavidBrunow Dec 8, 2019

DavidBrunow Dec 8, 2019 •

edited

	let resolver = resolver ?? resolverType.init(
	versionsForDependency: versions(for:),
	dependenciesForDependency: dependencies,
	resolvedGitReference: resolvedGitReference
	)

Add fetch-interval to outdated command to skip fetching the dependencies #2714

Are you sure you want to change the base?

Add fetch-interval to outdated command to skip fetching the dependencies #2714

Conversation

mollyIV commented Feb 24, 2019 • edited

Motivation and Context

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollyIV commented Apr 17, 2019

jdhealy commented Apr 24, 2019

Choose a reason for hiding this comment

mollyIV Apr 27, 2019 • edited

Choose a reason for hiding this comment

tmspzz commented May 20, 2019

mollyIV commented May 20, 2019

mollyIV commented May 21, 2019

stale bot commented Jun 20, 2019

mollyIV commented Jun 20, 2019

This comment has been minimized.

This comment has been minimized.

stale bot commented Aug 28, 2019

mollyIV commented Sep 2, 2019

stale bot commented Oct 2, 2019

mollyIV commented Oct 2, 2019

stale bot commented Nov 1, 2019

mollyIV commented Nov 1, 2019

stale bot commented Dec 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidBrunow Dec 8, 2019 • edited

Choose a reason for hiding this comment

Add fetch-interval to `outdated` command to skip fetching the dependencies #2714

Add fetch-interval to `outdated` command to skip fetching the dependencies #2714

mollyIV commented Feb 24, 2019 •

edited

mollyIV Apr 27, 2019 •

edited

DavidBrunow Dec 8, 2019 •

edited