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

Validate Carthage/Build content is up to date with Carthage.resolved #1222

Closed
baek-jinoo opened this issue Mar 28, 2016 · 22 comments

Comments

Projects
None yet
8 participants
@baek-jinoo
Copy link
Contributor

commented Mar 28, 2016

Context: I prefer to not include Carthage directory in the git repository (I cache the .framework files for my CI).

My goal is to allow our team members to be able to run the app while not having to worry about running carthage bootstrap if someone updates the dependency. For example, suppose we introduce a flag --check-cache. So, a command such as carthage bootstrap --check-cache could be added in the 'Build Phases' of the Xcode project and if the content of the 'Carthage/Build' directory is up to date with Carthage.resolved, it would result in a no-op. But, if the directory's content is stale when compared to Carthage.resolved, it would execute carthage bootstrap.

With this, I can foresee our development workflow improving in three ways:

  • If the version difference doesn't cause build errors, then developers will be doing development without the latest dependencies and find out if there is any issues when CI catches it. This is prevented.
  • If the build breaks when a dev fetches a change that causes any errors due to their stale Carthage folder, it will not break their initial build and keep working.
  • One less step to setup the project to run when setting up the project for the first time.

This approach is standard in other toolchains in other languages, where the build and run step's dependency step(s) checks the project dependencies as part of the flow. I believe this approach will be beneficial in this tool-chain as well.

I am willing to implement this change if you think it complies with Carthage's values. Then, I would like to get some input on how to check the staleness of the Build folder content.

@mdiep

This comment has been minimized.

Copy link
Member

commented Mar 28, 2016

So, a command such as carthage bootstrap --check-cache could be added in the 'Build Phases' of the Xcode project and if the content of the 'Carthage/Build' directory is up to date with Carthage.resolved, it would result in a no-op. But, if the directory's content is stale when compared to Carthage.resolved, it would execute carthage bootstrap.

How do you propose validating that it's up-to-date?

@gfontenot

This comment has been minimized.

Copy link

commented Mar 28, 2016

I've been doing this in our projects by copying Cartfile.resolved into Carthage and then running cmp Cartfile.resolved ./Carthage/Cartfile.resolved. It's super naive, but it works pretty well.

@mdiep

This comment has been minimized.

Copy link
Member

commented Mar 28, 2016

That will verify the checkout, but not the built products.

@baek-jinoo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

I posted this issue to pick your (@mdiep) brain on the approach to check the staleness of the frameworks in the Build folder. The source of truth is the .framework directories, but I don't see anything in there that's obvious to me to interpret a consistent means of determining the validity of the version of the Framework, especially when there might be slight differences in OS version, Xcode version and carthage version.

So, one approach that @bkobilansky and I can think of is to create a file that records the git sha of the dependency and the md5 checksum of the framework folder associated to it. This is to ensure that the framework directory is what was created with the file.

To go into a bit more detail, we could create a file (e.g. Nimble.version) beside the framework directories in the Build folder that has the git sha from where it originated from. This way, carthage could check to see if Nimble.version file exists and the content matches the git sha associated to the version of Nimble in the Cartfile.resolved file.

Lastly, we could also include the md5 of the Nimble.framework directory into Nimble.version so that the integrity of the framework is maintained in relation to Nimble.version. I think this is necessary for people checking in the Carthage folder to git while having different carthage versions between developers.

@mdiep

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

I like it. 👍

But I'd suggest a few tweaks:

  • Use the CarthageKit.PinnedVersion to identify the version. This is what's used in Cartfile.resolved. It's a commitish in git parlance, which is not exactly a SHA because it can also be something like a tag—anything that uniquely identifies a commit.
  • Use a SHA1 of the Nimble.framework directory to match what Git uses.
  • Prefix the version file with a . to make it hidden: .Nimble.version.
  • The flag should be --cache-builds and should default to true.

I agree that the .version file should include both the commitish and a hash of the built framework.

@baek-jinoo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

Assumptions:

  • CarthageKit.PinnedVersion is an abstraction over git commit
  • We would provide an option such as --no-cache-builds if someone wants carthage to ignore the existing .version file(s) but still create the .version file(s)
  • --(no-)cache-builds would only be relevant for carthage bootstrap
@mdiep

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

The flag would be relevant for update, bootstrap, and build.

@jgraves

This comment has been minimized.

Copy link

commented Mar 30, 2016

This would be immensely useful in our project as well. Is there active development happening in a branch for this? I'd love to contribute.

@baek-jinoo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2016

I am working on this on my fork. I will push up my branch and open a pull request when I get a chance. After I take a look, I'll identify work that can be done in parallel.

@baek-jinoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2016

Hey all. Just to update, I have been swamped with other tasks, but plan on finishing this as soon as I get more bandwidth. I'll surely ask more questions via PR when I get a good foothold on a possible solution.

@mz2

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

@baek-jinoo I would also love to see this, it'd speed up my workflow. If there are any chunks of work you could delegate & describe, do let me know, would be glad to help.

@baek-jinoo

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2016

I will clean up my branch a bit and list out my thoughts so anyone can pitch in. I'm super swamped right now, but I'll do this no later than this Thursday.

Jin

On May 23, 2016, at 6:38 AM, Matias Piipari notifications@github.com wrote:

@baek-jinoo I would also love to see this, it'd speed up my workflow. If there are any chunks of work you could delegate & describe, do let me know, would be glad to help.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@baek-jinoo

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2016

I cleaned it up a bit (with the time that I have) and pushed up the changes here. The spec skeleton is up, and beforeEach is implemented. I imported CryptoSwift to get the ball rolling, we should be able to use any SHA1 algorithm generation code. I am generating the SHA1 of the framework file itself.

Some TODOs identified:

  • Might want to copy out the SHA1 algorithm out so we don't put all the algorithms of CryptoSwift in the project
  • Might want to run the test from the Project object's buildCheckedOutDependenciesWithConfiguration rather than buildDependencyProject in Xcode file
  • Introduce command line argument and pass it all the way through
  • Finish implementing the part where it checks the commitish, SHA1 and flag to short circuit the build (make it less arrow-like using guard type check)
  • Connect the code that creates the version file

This is a handoff for someone to take over and finish it off unless my bandwidth opens up unexpectedly in the near future.

Please let me know if you need more clarification.

@marcvanolmen

This comment has been minimized.

Copy link

commented Jul 1, 2016

I'm also in need of this feature urgently, but I have some questions regarding, the chosen strategy here above:

a) don't understand why it is needed to SHA1 the framework.
b) I have learned that when you do not compile all the frameworks locally the .dsym files won't work properly: trying to step into a framework build by carthage will give you assembler code only. (so we stopped committing frameworks and .dsym files to our repository) So our Carthage folder is git ignored.
c) Also step (b) gave issues uploading .dsym files to Hockeyapp/Crashlytics with Swift libraries, because symbolizing crash logs failed sometimes for us.

My assumptions would be this:

  • Carthage folder will always be ignored in git..(although not important if you don't care about correct .dsym)
  • Carthage.resolved specifies which commit (version you have) you are expecting from the framework..
  • So when carthage finish building the Project Dependency, you store the following: Compiler Versions and Commit hash or version that was stored in carthage.resolved file.
  • Because it is not always easy to map the Project Dependency name with a framework name. (you have to analyze the Xcode project file for that)

If Carthage.resolved has this:

github "SnapKit/Masonry" "v0.6.3"
github "MyFork/YLMoment" "017a5a939c4332ee62fda83cd211b0849b4fe4de"

then cached-build.resolved file for each platform (for example in Carthage/Build/iOS) will have this:

github "SnapKit/Masonry" "v0.6.3", Swift 2.2, Clang 5.1.1
github "MyFork/YLMoment" "017a5a939c4332ee62fda83cd211b0849b4fe4de", Swift 2.2, Clang 5.1.1
  • In case for the platform there was no target specified we store NULL or some indicator for that.

So for -cache-builds checking you do the following:

  • check if the version is the same or is NULL
  • Check if the Compiler is the same as the current version of Xcode you are using.

For CI:

a) Job folder is preserved between next run: Never delete the Carthage folder and it will then only compile in case something was changed, Also in case you have many Jenkins jobs you can always store the Carthage in a common folder shared by all the Jenkins/Fastlane jobs.
b) Job folder is destroyed after every run: use some stored version of the carthage folder and make sure it contains the "cached-build.resolved" file and after copying verify if ok otherwise rebuild, etc...

For each developer:

  • we fall under the CI case (a) case.

Any thoughts?

@mdiep

This comment has been minimized.

Copy link
Member

commented Jul 2, 2016

don't understand why it is needed to SHA1 the framework.

To ensure that the framework is the one that was built with Carthage.

@baek-jinoo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2016

don't understand why it is needed to SHA1 the framework.

To ensure that the framework is the one that was built with Carthage with a version that updates the .version file. An older version of Carthage might not know to update the .version file and only updates the framework. This could happen for when the Carthage folder is not ignored by the VCS e.g. git.

For the majority of use-case of users associated with Carthage, having an Xcode build string should be sufficient to ensure that the correct version of swift was used. For example, the "7D1014" in "Version 7.3.1 (7D1014)".

But, for when we do have ABI stability in the future, we should include the swift version starting now so it's easy for us to have a demarcation of when we can start ignoring the Xcode build string for users with ABI-stable compiler version.

I don't see the purpose of including an indicator or NULL for a non-existent target. Can you elaborate on that?

In terms of the clang version, I'm not sure why that is needed, can you elaborate on that as well?

Also, while this is being built, you could use carthage_cache gem in Jenkins to just download the frameworks. It's a decent workaround solution, and you can easily script it in a way so you can let the build process handle creating a new version of the cache and upload it to s3 when the Cartfile.resolved changes.

@marcvanolmen

This comment has been minimized.

Copy link

commented Jul 2, 2016

Thank you guys!

@baek-jinoo > To ensure that the framework is the one that was built with Carthage

If that would be the only case isn't it sufficient to include the carthage version that was used, and trigger a specific error if that's the case: Tell the users that machine/repo needs to be upgraded to latest carthage.

@mdiep >To ensure that the framework is the one that was built with Carthage.

I assume you are thinking about this scenario, somebody manually run an Xcode command to update the framework in carthage (after maybe updating source code)? If that's the case we have to maybe trigger a special error case (I'm not sure carthage has protection for this now already) and ask the user to manually correct this, to make sure we don't wipe out any source code changes he did.

For the majority of use-case of users associated with Carthage, having an Xcode build string should be sufficient to ensure that the correct version of swift was used. For example, the "7D1014" in "Version 7.3.1 (7D1014)".

Probably you are right on this, I was thinking ahead when both Swift 2.3 and Swift 3.0 would be installed in Xcode 8.0, but even that would be the case the Xcode version would be enough because this would need a commit change anyway for the changing the project from 2.3 to 3.0 in Xcode 8.0

But I was thinking more about an edge case where somebody set up an "Alternative Toolchain support": https://developer.apple.com/library/prerelease/content/documentation/ToolsLanguages/Conceptual/Xcode_Overview/AlternativeToolchains.html
In that scenario Xcode version would be the same but Swift compiler different.

I don't see the purpose of including an indicator or NULL for a non-existent target. Can you elaborate on that?

We have both iOS and tvOS targets in our Project, that share one carthage file. When we run carthage command with platform iOS, tvOS, but some of our dependent projects don't have targets for tvOS targets, so don't produce any frameworks. In that scenario: How will you know if that framework was just forgotten to be build or there was no framework at all for that dependent project? That's why I propose that we have to register somewhere that the we attempted to find a target for that platform (tvOS) but couldn't find anything for it.

In terms of the clang version, I'm not sure why that is needed, can you elaborate on that as well?

Only case is alternative toolchain, but not sure it can contain also updated clang.

you could use carthage_cache gem

Thanks didn't know about this one. Because we never modify anything inside the carthage folder and we don't wipe out our Jenkins jobs on our internal build machine probably will modify the ruby script mentioned here: #924 and add it as our first build phase in our Xcode project.

@mdiep

This comment has been minimized.

Copy link
Member

commented Jul 2, 2016

How will you know if that framework was just forgotten to be build or there was no framework at all for that dependent project?

Carthage can load that information from the build settings.

Or, more clearly: Carthage generates a sequence of projects to build and frameworks from irrelevant platforms aren't included.

@baek-jinoo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2016

If that would be the only case isn't it sufficient to include the carthage version that was used, and trigger a specific error if that's the case: Tell the users that machine/repo needs to be upgraded to latest carthage.

I am not sure it adds more value to restrict Carthage version to ensure that they don't blow away the cache if it will still compile and allows them to use the output without issue. I haven't used this yet, but this might help if you want to restrict Carthage version with brew.

I assume you are thinking about this scenario, somebody manually run an Xcode command to update the framework in carthage (after maybe updating source code)? If that's the case we have to maybe trigger a special error case (I'm not sure carthage has protection for this now already) and ask the user to manually correct this, to make sure we don't wipe out any source code changes he did.

My thought on this is that users should assume that the content within the Carthage folder is owned and manipulated by Carthage. If they go in there muck with the binaries via manual steps, they should be going in with an understand that they're interfering with Carthage and be OK if Carthage takes over when they execute a Carthage command. If a user wants to manually manage dependencies that are not driven by Cartfiles, they should probably do it outside of the Carthage folder.

In that scenario Xcode version would be the same but Swift compiler different.

I did think about the alternative toolchain, but I thought it would still be an edge case. However, if we do want to support this, I would assume that having the swift version should be sufficient for this. I am not sure exactly how much flexibility will be provided by the alternative toolchain, but that would probably determine how much information we would need to ensure the relevance of the cache.

@jasonboyle

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

I am working with @BobElDevil to continue @baek-jinoo's work in my fork.

@Dschee

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

I just want to point out that you can find the current state of implementation to close this issue in #1489 – there are quite a few references here, so I thought I'd mention this explicitly. :)

@mdiep

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

#1489 has been merged and will be released soon in Carthage 0.20.

@mdiep mdiep closed this Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.