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

Remove query parameters from repo name parsed from binary Cartfile entry #222

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

ffittschen
Copy link
Contributor

@ffittschen ffittschen commented Apr 19, 2020

In case a Binary entry in the Cartfile has a query parameter, Rome will consider this query parameters as part of the repo name used in the cache, which results in this binary entry not being able to be cached, because the version file doesn't have query parameters in the filename.

To resolve this issue I changed the gitRepoNameFromCartfileEntry method to not only replace the .json file extension, but to split on it and use the head.

Reproduction Steps:

  1. carthage bootstrap using the Cartfile.resolved from below
  2. rome upload
  3. Clear the local Carthage/Build folder
  4. rome download

Expected behavior:

All dependencies are being fetched from the cache.

Observed behavior:

Rome cannot find the version file, because it looks for the wrong file name.

Problematic Romefile

Note: Using tealium-carthage-plcrashreporter?_cb=2 in the Romefile will result in the framework and dSYM being uploaded and downloaded, but the version file will not be uploaded or downloaded. Using tealium-carthage-plcrashreporter in the Romefile won't work, because then executing rome list --missing will output tealium-carthage-plcrashreporter?_cb=2 1.4.0

cache:
  local: ~/Library/Caches/Rome

repositoryMap:
- tealium-carthage-plcrashreporter?_cb=2:
  - name: TealiumCrashReporteriOS
- tealium-swift:
  - name: TealiumAppData
  - name: TealiumAttribution
  - name: TealiumAutotracking
  - name: TealiumCollect
  - name: TealiumConnectivity
  - name: TealiumConsentManager
  - name: TealiumCore
  - name: TealiumCrash
  - name: TealiumDelegate
  - name: TealiumDeviceData
  - name: TealiumDispatchQueue
  - name: TealiumLifecycle
  - name: TealiumLocation
  - name: TealiumLogger
  - name: TealiumPersistentData
  - name: TealiumRemoteCommands
  - name: TealiumTagManagement
  - name: TealiumVisitorService
  - name: TealiumVolatileData

Problematic Cartfile.resolved

binary "https://tags.tiqcdn.com/dle/tealiummobile/tealium-ios-carthage/tealium-carthage-plcrashreporter.json?_cb=2" "1.4.0"
github "tealium/tealium-swift" "1.9.4"

Rome version: 0.23.2.63 - Romam uno die non fuisse conditam.
OS and version: macOS 10.15.4 (19E266)

Additional information:

  • Problem started happening recently, didn't happen in an older version of Rome: No
  • Problem can be reliably reproduced, doesn't happen randomly: Yes
  • Problem happens with all Romefile / Cartfile.resolved and projects, not only some files or projects: No

@hlintBot
Copy link

hlintBot commented Apr 19, 2020

4 Warnings
⚠️ src/Utils.hs#L337 - Found Use tuple-section

\ f -> (f, g)

Why Not

(, g)

    
⚠️ tests/Tests.hs#L110 - Found Use uncurry

\ (a, b) -> a || b

Why Not

uncurry (||)

    
⚠️ tests/Tests.hs#L125 - Found Redundant bracket

(unProjectName (_projectName r)) ++ " = " ++ f

Why Not

unProjectName (_projectName r) ++ " = " ++ f

    
⚠️ tests/Tests.hs#L126 - Found Redundant bracket

(map show) (_frameworks r)

Why Not

map show (_frameworks r)

    

Generated by 🚫 Danger

Copy link
Owner

@tmspzz tmspzz left a comment

Choose a reason for hiding this comment

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

Would be nice to extract this and add a test

@@ -148,6 +149,17 @@ prop_filterRomeFileEntriesByPlatforms_min :: [RomefileEntry] -> [RomefileEntry]
prop_filterRomeFileEntriesByPlatforms_min base filteringValues =
(length $ base `filterRomeFileEntriesByPlatforms` filteringValues) <= length base

Choose a reason for hiding this comment

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

⚠️ Found Move brackets to avoid $

(length $ base `filterRomeFileEntriesByPlatforms` filteringValues)
  <= length base

Why Not

length (base `filterRomeFileEntriesByPlatforms` filteringValues) <=
  length base

    

@tmspzz tmspzz merged commit 4584974 into tmspzz:master Apr 29, 2020
@tmspzz tmspzz added the bug label May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants