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

--cache-builds will skip building even if a commit different to what's been built is locally available #1785

Open
mz2 opened this Issue Feb 23, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@mz2
Contributor

mz2 commented Feb 23, 2017

The new --cache-builds behaviour is not behaving at least to my expectation if I use it together with --use-submodules for a local package editing workflow. Here's a demonstration:

  1. Build your dependent package X: carthage build --cache-builds X where X is a package that's been pulled in with --use-submodules to Carthage/Checkouts/X.
  2. Make local changes to X and commit them to the submodule at Carthage/Checkouts/X.
  3. Repeat building X: carthage build --cache-builds X – observe that X is skipped building and the commit from the local checkout was not read, presumably because the comparison is only done against the commitish in Cartfile.resolved.

I would argue this is a bug, because carthage build without --cache-builds will in fact build the current revision from Carthage/Checkouts/X. Requiring the editing of Cartfile.resolved as part of a local package editing workflow would be both illogical (leaks the implementation detail of what keys are used to invalidate the cache) and actually kind of dangerous because you may end up committing by accident for example changes to Cartfile.resolved that have intentionally not been pushed out yet.

  • carthage version: current head of master (9f1cc9e).
  • xcodebuild -version: Xcode 8.2.1 (8C1002)
  • Are you using --no-build? no
  • Are you using --no-use-binaries? no
  • Are you using --use-submodules? yes
@BobElDevil

This comment has been minimized.

Show comment
Hide comment
@BobElDevil

BobElDevil Feb 23, 2017

Contributor

I think it would make sense to compare against the current 'subproject commit' value. That way we could also detect 'dirty' working trees as well and rebuild. (though with 'dirty' trees we would have to make sure that commit-dirty != commit-dirty).

Non submodule usage still won't be able to detect changes to the working copy, not sure if we want to get into that game. It might do to just say "if you're not using submodules you shouldn't be touching the Checkouts folder"

Contributor

BobElDevil commented Feb 23, 2017

I think it would make sense to compare against the current 'subproject commit' value. That way we could also detect 'dirty' working trees as well and rebuild. (though with 'dirty' trees we would have to make sure that commit-dirty != commit-dirty).

Non submodule usage still won't be able to detect changes to the working copy, not sure if we want to get into that game. It might do to just say "if you're not using submodules you shouldn't be touching the Checkouts folder"

@mz2

This comment has been minimized.

Show comment
Hide comment
@mz2

mz2 Feb 23, 2017

Contributor

Non submodule usage still won't be able to detect changes to the working copy, not sure if we want to get into that game. It might do to just say "if you're not using submodules you shouldn't be touching the Checkouts folder"

With or without this particular feature, I think that's a pretty reasonable guidance to give.

Contributor

mz2 commented Feb 23, 2017

Non submodule usage still won't be able to detect changes to the working copy, not sure if we want to get into that game. It might do to just say "if you're not using submodules you shouldn't be touching the Checkouts folder"

With or without this particular feature, I think that's a pretty reasonable guidance to give.

@scottrhoyt

This comment has been minimized.

Show comment
Hide comment
@scottrhoyt

scottrhoyt Feb 23, 2017

Contributor

I think --use-cache should be incompatible with --use-submodules for the time being. The guidance should remain that modifying Checkouts without --use-submodules results in undefined behavior. What do you think about that?

Perhaps a PR can be opened to implement that?

Contributor

scottrhoyt commented Feb 23, 2017

I think --use-cache should be incompatible with --use-submodules for the time being. The guidance should remain that modifying Checkouts without --use-submodules results in undefined behavior. What do you think about that?

Perhaps a PR can be opened to implement that?

@mz2

This comment has been minimized.

Show comment
Hide comment
@mz2

mz2 Feb 23, 2017

Contributor

Why exactly should those be incompatible?

Contributor

mz2 commented Feb 23, 2017

Why exactly should those be incompatible?

@scottrhoyt

This comment has been minimized.

Show comment
Hide comment
@scottrhoyt

scottrhoyt Feb 23, 2017

Contributor

While I think this feature eventually could/should support submodules, my concern is delaying the release for users than don't use submodules. I'd rather field the 80% solution now.

--use-submodules users won't have their current behavior changed. And since one of the major use cases for submodules is to change dependencies in step with a main project, the value of caching is lower than for normal users.

That being said, I could be overestimating the time needed to fix this, review, and accept it or underestimating the same for a PR that disables caching with --use-submodules.

Contributor

scottrhoyt commented Feb 23, 2017

While I think this feature eventually could/should support submodules, my concern is delaying the release for users than don't use submodules. I'd rather field the 80% solution now.

--use-submodules users won't have their current behavior changed. And since one of the major use cases for submodules is to change dependencies in step with a main project, the value of caching is lower than for normal users.

That being said, I could be overestimating the time needed to fix this, review, and accept it or underestimating the same for a PR that disables caching with --use-submodules.

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Feb 24, 2017

Member

I'd love to see a PR that does one of the following:

  1. Documents this limitation
  2. Uses the solution outlined by @BobElDevil above
  3. Disables --use-cache when --use-submodules is specified
Member

mdiep commented Feb 24, 2017

I'd love to see a PR that does one of the following:

  1. Documents this limitation
  2. Uses the solution outlined by @BobElDevil above
  3. Disables --use-cache when --use-submodules is specified
@mz2

This comment has been minimized.

Show comment
Hide comment
@mz2

mz2 Feb 24, 2017

Contributor

If you @mdiep or @BobElDevil can give some pointers towards the point 2 I am happy to give it a go:

  • Hopefully there a single place to change for the commitish comparison against the value in the version file?
  • What is the API needed to get the subproject commit value?
Contributor

mz2 commented Feb 24, 2017

If you @mdiep or @BobElDevil can give some pointers towards the point 2 I am happy to give it a go:

  • Hopefully there a single place to change for the commitish comparison against the value in the version file?
  • What is the API needed to get the subproject commit value?
@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Feb 25, 2017

Member

Most of it should be contained in VersionFile.swift.

git rev-parse HEAD gets the current commit. That can be executed in a submodule to get the current commit for that submodule. I believe a Carthage wrapper for rev-parse already exists.

Member

mdiep commented Feb 25, 2017

Most of it should be contained in VersionFile.swift.

git rev-parse HEAD gets the current commit. That can be executed in a submodule to get the current commit for that submodule. I believe a Carthage wrapper for rev-parse already exists.

Kuniwak added a commit to Kuniwak/RxNextExample that referenced this issue Dec 18, 2017

Install RxSwift and R.swift
We cannot use --use-submodules with --cache-builds,
so dependencies do not become submodules. See
Carthage/Carthage#1785
@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Jun 30, 2018

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.

stale bot commented Jun 30, 2018

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.

@stale stale bot added the stale label Jun 30, 2018

@stale stale bot closed this Jul 7, 2018

@jdhealy jdhealy added help wanted and removed stale labels Jul 26, 2018

@jdhealy jdhealy reopened this Jul 26, 2018

@mdiep mdiep added the bug label Jul 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment