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

[Dependencies.swift] Fix failed decoding for not present container key + Tests #3164

Merged

Conversation

havebeenfitz
Copy link
Contributor

@havebeenfitz havebeenfitz commented Jul 7, 2021

Resolves #3107 (comment)

Short description πŸ“

Is seems like Carthage version files are not so strict, so I made the container key optional and wrote a test for this kind of case. I'm not sure if this shoud be as 1.45.2 or 1.46 and newer, please check the merge destination

Checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

@havebeenfitz havebeenfitz changed the title fix: fix failed decoding for not present container key + Tests [Dependencies.swift] Fix failed decoding for not present container key + Tests Jul 7, 2021
Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this one @havebeenfitz πŸ‘πŸΌ. Besides the tiny comment that I left, it'd be great to update the CHANGELOG.md to list your change there. After that and if CI is green, we can merge :)

@pepicrft
Copy link
Contributor

pepicrft commented Jul 8, 2021

@all-contributors add @havebeenfitz for bug

@allcontributors
Copy link
Contributor

@pepibumur

I've put up a pull request to add @havebeenfitz! πŸŽ‰

@havebeenfitz
Copy link
Contributor Author

So it looks like binary Carthage dependencies deliberately lack the container key. And Graph relies on that, otherwise, dependency is absent for Graph

At first I thought ok, let’s rely on name + .xcframeworkextension, but then I looked at .binary carthage dependencies and they all lack the container
So I guess ThirdPartyDependency and ThirdPartyDependency.Kind should have one more case - framework and parse accordingly

What do you think?

.GoogleMaps.version

{
  "Mac" : [
  ],
  "watchOS" : [
  ],
  "tvOS" : [
  ],
  "commitish" : "3.10.0",
  "iOS" : [
    {
      "name" : "GoogleMaps",
      "hash" : "87a19f3ade1a24cc17b9c943952326b3b237b6205265054da33350311946dc0f",
      "linking" : "dynamic"
    },
    {
      "name" : "GoogleMapsBase",
      "hash" : "19884971db68d9d66ef45134fed7890b27200153c90e69022db54e1d6549b57d",
      "linking" : "dynamic"
    },
    {
      "name" : "GoogleMapsCore",
      "hash" : "42eb96fd4a53f638f56befc6422cf6d07165d9d1e8569c934bf63a17c583a77d",
      "linking" : "dynamic"
    },
    {
      "name" : "GoogleMapsM4B",
      "hash" : "780bc4e295f5d2adba95756a10f9c0cbbd5e3908f7142ef4ce7c81745d1f9fdb",
      "linking" : "dynamic"
    }
  ]
}

Copy link
Collaborator

@laxmorek laxmorek left a comment

Choose a reason for hiding this comment

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

I will review the rest after my working hours. :)

CHANGELOG.md Outdated Show resolved Hide resolved
@laxmorek
Copy link
Collaborator

laxmorek commented Jul 8, 2021

@havebeenfitz call ./fourier format swift --fix to satisfy formatting rules

You can read more about fourier here.

@laxmorek
Copy link
Collaborator

laxmorek commented Jul 8, 2021

@tuist/core Do you know why CI doest fire checks for unit and acceptance tests?

@havebeenfitz havebeenfitz force-pushed the bugfix/fix-failed-dependencies-fetch branch 2 times, most recently from 57c9003 to 51c2e5a Compare July 8, 2021 15:25
@havebeenfitz
Copy link
Contributor Author

Hey guys! I spent some time fixing this Graph and realized that it's starting to look like a feature more than a bugfix. And while graph feature is definitely great and useful, it seems like it's less used and doesn't produce error now, just works a bit incorrectly.

Assume that we have multiple platform in Dependencies.swift and we're importing carthage dependencies as binaries. Now it will just hide the framework dependency. I will add support for framework dependencies in graph just a bit later in separate PR if you don't mind

@havebeenfitz havebeenfitz force-pushed the bugfix/fix-failed-dependencies-fetch branch from 51c2e5a to dc6f2a5 Compare July 8, 2021 15:28
Copy link
Collaborator

@laxmorek laxmorek left a comment

Choose a reason for hiding this comment

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

I left a few minor comments. I think it is fine to push this fix into main and add framework support later.

@havebeenfitz havebeenfitz force-pushed the bugfix/fix-failed-dependencies-fetch branch from 5b72d65 to 46efac8 Compare July 9, 2021 08:01
CHANGELOG.md Outdated Show resolved Hide resolved
@havebeenfitz havebeenfitz force-pushed the bugfix/fix-failed-dependencies-fetch branch from e00947c to 6009c31 Compare July 9, 2021 12:49
@laxmorek laxmorek merged commit 009e634 into tuist:main Jul 9, 2021
@havebeenfitz havebeenfitz deleted the bugfix/fix-failed-dependencies-fetch branch July 10, 2021 15:28
laxmorek added a commit that referenced this pull request Jul 12, 2021
* [fix-styling] remove - `ThirdPartyDependency.framework`

* [fix-styling] change - `logger.info` -> `logger.warning`
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.

πŸ“¦ [v1.45.0][Dependencies.swift] The tuist dependencies fetch command fails.
4 participants