[WIP] Add Support for XCFrameworks by evandcoleman · Pull Request #244 · tmspzz/Rome

evandcoleman · 2021-04-15T22:23:33Z

Addresses #238

A first pass at adding XCFramework support to Rome. This is my first time writing any Haskell so please be kind. This is not a complete implementation, but it's in a place where it's usable. I'd love if someone with more Haskell experience could pick this up and bring it across the finish line. Or maybe even just point me in the right direction.

Changes

Added a --use-xcframeworks flag to the upload, download, and list commands

Known Issues

Running with --use-xcframeworks and a --platform ignores frameworks that aren't xcframeworks (workaround is to run it twice with and without the use xcframeworks option)
Running with --use-xcframeworks and a --platform places all frameworks inside a platform specific folder in the cache

P.S: We love Rome at The Knot. It's undoubtedly saved us hundreds (if not thousands) of developer hours in the two years we've been using it. Thank you so much for building such a useful tool.

hlintBot · 2021-04-15T22:34:00Z

          listMode
+          (_useXcFrameworks useXcFrameworksFlag)
          (reverseRepositoryMap <> if _noSkipCurrent noSkipCurrentFlag then currentInvertedMap else M.empty)
          (frameworkVersions <> if _noSkipCurrent noSkipCurrentFlag


⚠️ Found Redundant bracket

if _noSkipCurrent noSkipCurrentFlag then (currentFrameworkVersions `filterOutFrameworksAndVersionsIfNotIn` finalIgnoreNames) else []

Why Not

if _noSkipCurrent noSkipCurrentFlag then currentFrameworkVersions `filterOutFrameworksAndVersionsIfNotIn` finalIgnoreNames else []

hlintBot · 2021-04-15T22:34:01Z


-  frameworkNameWithFrameworkExtension = appendFrameworkExtensionTo f
-  platformBuildDirectory              = carthageArtifactsBuildDirectoryForPlatform platform f
+  frameworkNameWithFrameworkExtension =


⚠️ Found Reduce duplication

frameworkNameWithFrameworkExtension = if useXcFrameworks then appendXcFrameworkExtensionTo f else appendFrameworkExtensionTo f platformBuildDirectory = if useXcFrameworks then carthageBuildDirectory else carthageArtifactsBuildDirectoryForPlatform platform f frameworkDirectory = platformBuildDirectory </> frameworkNameWithFrameworkExtension dSYMNameWithDSYMExtension = frameworkNameWithFrameworkExtension <> ".dSYM" dSYMdirectory = platformBuildDirectory </> dSYMNameWithDSYMExtension bcSymbolMapPath d = platformBuildDirectory </> bcsymbolmapNameFrom d

Why Not

Combine with src/Lib.hs:928:3

hlintBot · 2021-04-15T22:34:02Z


-  frameworkNameWithFrameworkExtension = appendFrameworkExtensionTo f
-  platformBuildDirectory              = carthageArtifactsBuildDirectoryForPlatform platform f
+  frameworkNameWithFrameworkExtension =


⚠️ Found Reduce duplication

frameworkNameWithFrameworkExtension = if useXcFrameworks then appendXcFrameworkExtensionTo f else appendFrameworkExtensionTo f platformBuildDirectory = if useXcFrameworks then carthageBuildDirectory else carthageArtifactsBuildDirectoryForPlatform platform f frameworkDirectory = platformBuildDirectory </> frameworkNameWithFrameworkExtension dSYMNameWithDSYMExtension = frameworkNameWithFrameworkExtension <> ".dSYM" dSYMdirectory = platformBuildDirectory </> dSYMNameWithDSYMExtension bcSymbolMapPath d = platformBuildDirectory </> bcsymbolmapNameFrom d

Why Not

Combine with src/Lib.hs:829:3

hlintBot · 2021-04-15T22:34:02Z

              e2 <- runExceptT $ do
-                frameworkBinary <- getFrameworkFromS3 s3BucketName reverseRomeMap fVersion platform
+                frameworkBinary <- getFrameworkFromS3 s3BucketName useXcFrameworks reverseRomeMap fVersion platform
                saveBinaryToLocalCache lCacheDir frameworkBinary (prefix </> remoteFrameworkUploadPath) fwn verbose


⚠️ Found Reduce duplication

saveBinaryToLocalCache lCacheDir frameworkBinary (prefix </> remoteFrameworkUploadPath) fwn verbose deleteFrameworkDirectory fVersion platform verbose unzipBinary frameworkBinary fwn frameworkZipName verbose <* ifExists frameworkExecutablePath (makeExecutable frameworkExecutablePath)

Why Not

Combine with src/Lib.hs:1220:17

hlintBot · 2021-04-15T22:34:03Z

 where
-  frameworkZipName          = frameworkArchiveName f version
-  remoteFrameworkUploadPath = remoteFrameworkPath platform reverseRomeMap f version
+  frameworkZipName          = frameworkArchiveName f version useXcFrameworks


⚠️ Found Reduce duplication

frameworkZipName = frameworkArchiveName f version useXcFrameworks remoteFrameworkUploadPath = remoteFrameworkPath useXcFrameworks platform reverseRomeMap f version remoteBcSymbolmapUploadPathFromDwarf dwarfUUID = remoteBcsymbolmapPath dwarfUUID platform reverseRomeMap f version dSYMZipName = dSYMArchiveName f version remotedSYMUploadPath = remoteDsymPath platform reverseRomeMap f version platformBuildDirectory = carthageArtifactsBuildDirectoryForPlatform platform f dSYMName = fwn <> ".dSYM" frameworkExecutablePath = frameworkBuildBundleForPlatform platform f </> fwn

Why Not

Combine with src/Lib.hs:1278:3

hlintBot · 2021-04-15T22:34:04Z

	18 Warnings
⚠️	PR is classed as Work in Progress
⚠️	src/Caches/Local/Downloading.hs#L187 - Found Reduce duplication `frameworkNameWithFrameworkExtension = appendFrameworkExtensionTo f platformBuildDirectory = carthageArtifactsBuildDirectoryForPlatform platform f frameworkDirectory = platformBuildDirectory </> frameworkNameWithFrameworkExtension` Why Not `Combine with src/Caches/Local/Downloading.hs:215:3`
⚠️	src/Caches/Local/Downloading.hs#L207 - Found Use tuple-section `\ e -> (dwarfUUID, e)` Why Not `(dwarfUUID,)`
⚠️	src/Caches/S3/Downloading.hs#L189 - Found Use tuple-section `\ e -> (dwarfUUID, e)` Why Not `(dwarfUUID,)`
⚠️	src/CommandParsers.hs#L98 - Found Use <$> `pure Upload <*> udcPayloadParser` Why Not `Upload <$> udcPayloadParser`
⚠️	src/CommandParsers.hs#L101 - Found Use <$> `pure Download <*> udcPayloadParser` Why Not `Download <$> udcPayloadParser`
⚠️	src/Engine/Downloading.hs#L141 - Found Use tuple-section `\ e -> (dwarfUUID, e)` Why Not `(dwarfUUID,)`
⚠️	src/Lib.hs#L239 - Found Reduce duplication let finalRepositoryMapEntries = if _noIgnore noIgnoreFlag then repositoryMapEntries else repositoryMapEntries `filterRomeFileEntriesByPlatforms` ignoreMapEntries let repositoryMap = toRepositoryMap finalRepositoryMapEntries let reverseRepositoryMap = toInvertedRepositoryMap finalRepositoryMapEntries let finalIgnoreNames = if _noIgnore noIgnoreFlag then [] else ignoreFrameworks Why Not `Combine with src/Lib.hs:323:5`
⚠️	src/Lib.hs#L248 - Found Reduce duplication let filteredCurrentMapEntries = currentMapEntries `filterRomeFileEntriesByPlatforms` ignoreMapEntries let currentFrameworks = concatMap (snd . romeFileEntryToTuple) filteredCurrentMapEntries let currentFrameworkVersions = map (flip FrameworkVersion currentVersion) currentFrameworks let currentInvertedMap = toInvertedRepositoryMap filteredCurrentMapEntries Why Not `Combine with src/Lib.hs:347:17`
⚠️	src/Lib.hs#L977 - Found Reduce duplication `saveBinaryToLocalCache lCacheDir versionFileBinary (prefix </> versionFileRemotePath) versionFileName verbose liftIO $ saveBinaryToFile versionFileBinary versionFileLocalPath sayFunc $ "Copied " <> versionFileName <> " to: " <> versionFileLocalPath` Why Not `Combine with src/Lib.hs:1349:15`
⚠️	src/Lib.hs#L988 - Found Reduce duplication `versionFileName = versionFileNameForProjectName $ fst projectNameAndVersion versionFileLocalPath = carthageBuildDirectory </> versionFileName versionFileRemotePath = remoteVersionFilePath projectNameAndVersion` Why Not `Combine with src/Lib.hs:1360:3`
⚠️	src/Lib.hs#L1084 - Found Reduce duplication `let symbolmapLoggingName = fwn <> "." <> bcsymbolmapNameFrom dwarfUUID let bcsymbolmapZipName d = bcsymbolmapArchiveName d version let localBcsymbolmapPathFrom d = platformBuildDirectory </> bcsymbolmapNameFrom d` Why Not `Combine with src/Lib.hs:1239:17`
⚠️	src/Lib.hs#L1088 - Found Reduce duplication saveBinaryToLocalCache lCacheDir symbolmapBinary (prefix </> remoteBcSymbolmapUploadPathFromDwarf dwarfUUID) fwn verbose deleteFile (localBcsymbolmapPathFrom dwarfUUID) verbose `catch` (\ e -> if isDoesNotExistError e then when verbose $ sayFunc ("Error :" <> displayException e) else throwM e) unzipBinary symbolmapBinary symbolmapLoggingName (bcsymbolmapZipName dwarfUUID) verbose Why Not `Combine with src/Lib.hs:1243:17`
⚠️	src/Lib.hs#L1117 - Found Reduce duplication `saveBinaryToLocalCache lCacheDir dSYMBinary (prefix </> remotedSYMUploadPath) dSYMName verbose deleteDSYMDirectory fVersion platform verbose unzipBinary dSYMBinary dSYMName dSYMZipName verbose` Why Not `Combine with src/Lib.hs:1271:17`
⚠️	src/Lib.hs#L1151 - Found Use lambda-case `\ e -> case e of ErrorGettingDwarfUUIDs -> sayFunc $ "Error: Cannot retrieve symbolmaps ids for " <> fwn (FailedDwarfUUIDs dwardUUIDsAndErrors) -> mapM_ (sayFunc . snd) dwardUUIDsAndErrors` Why Not `\case ErrorGettingDwarfUUIDs -> sayFunc $ "Error: Cannot retrieve symbolmaps ids for " <> fwn (FailedDwarfUUIDs dwardUUIDsAndErrors) -> mapM_ (sayFunc . snd) dwardUUIDsAndErrors`
⚠️	src/Lib.hs#L1303 - Found Use lambda-case `\ e -> case e of ErrorGettingDwarfUUIDs -> sayFunc $ "Error: Cannot retrieve symbolmaps ids for " <> fwn (FailedDwarfUUIDs dwardUUIDsAndErrors) -> mapM_ (sayFunc . snd) dwardUUIDsAndErrors` Why Not `\case ErrorGettingDwarfUUIDs -> sayFunc $ "Error: Cannot retrieve symbolmaps ids for " <> fwn (FailedDwarfUUIDs dwardUUIDsAndErrors) -> mapM_ (sayFunc . snd) dwardUUIDsAndErrors`
⚠️	src/Types/Commands.hs#L28 - Found Use newtype instead of data `data RomeUtilsPayload = RomeUtilsPayload{_subcommand :: RomeUtilsSubcommand} deriving (Show, Eq)` Why Not `newtype RomeUtilsPayload = RomeUtilsPayload{_subcommand :: RomeUtilsSubcommand} deriving (Show, Eq)`
⚠️	src/Utils.hs#L345 - Found Use tuple-section `\ f -> (f, g)` Why Not `(, g)`

Generated by 🚫 Danger

tmspzz · 2021-04-22T13:58:14Z

@evandcoleman I missed this, will review asap

tmspzz · 2021-04-22T14:22:04Z

@evandcoleman can you elaborate a bit more on the use of --platforms with .xcframeworks?

Does it even make sense to allow both flags? Why not just error out if both --platforms and --use-xcframeworks are specified.

tmspzz · 2021-04-22T14:05:59Z

                        , SkipLocalCacheFlag {- skipLocalCache -}
-                                            , NoSkipCurrentFlag {- noSkipCurrentFlag -}
-                                                               , ConcurrentlyFlag) {- concurrentlyFlag -}
+                                            , NoSkipCurrentFlag {- skipLocalCache -}


this looks like a weird rename. This is supposed to skip the current framework. Not the local cache. Unless I made a mistake that is.

evandcoleman · 2021-04-25T14:06:20Z

@evandcoleman can you elaborate a bit more on the use of --platforms with .xcframeworks?

Does it even make sense to allow both flags? Why not just error out if both --platforms and --use-xcframeworks are specified.

I think we'll want to support including both flags. In the case of Carthage frameworks that are closed-source and distributed as a compiled binary, those still get placed in a platform specific folder in the Carthage build folder.

tmspzz · 2021-04-26T08:30:39Z

@evandcoleman can you elaborate a bit more on the use of --platforms with .xcframeworks?
Does it even make sense to allow both flags? Why not just error out if both --platforms and --use-xcframeworks are specified.

I think we'll want to support including both flags. In the case of Carthage frameworks that are closed-source and distributed as a compiled binary, those still get placed in a platform specific folder in the Carthage build folder.

But that only makes sense for regular frameworks. Carthage puts .xcframeworks outside of platform folders if I recall correctly.

evandcoleman · 2021-04-26T14:09:33Z

Carthage puts .xcframeworks outside of platform folders if I recall correctly.

Yup! That is the case, but I'm thinking of frameworks such as the Braze SDK. Their Carthage framework is only distributed as a prebuilt (.framework) binary. Without the platform flag and with the xcframework flag, should Rome fail in that case? Or maybe fallback to looking for a regular framework and assume we want all platforms?

tmspzz · 2021-04-28T19:30:53Z

Without the platform flag and with the xcframework flag, should Rome fail in that case?

I think it should fail. Either it finds and xcframeworks or it doesn't. No fallbacks.

damiankolasinski · 2021-05-17T20:52:49Z

Any update here guys? Can't wait for this one 😄 @tmspzz @evandcoleman

mpdifran · 2021-06-25T14:28:48Z

@evandcoleman @tmspzz Anything we can do to help get this in, even in an MVP form?

jangelsb · 2021-07-07T18:19:51Z

Super excited for this! Let us know if there is anything we can do to help

jesus-mg-ios · 2021-08-02T15:44:02Z

@tmspzz @evandcoleman

evandcoleman · 2021-08-03T13:41:09Z

Unfortunately, I don't really have the time to spend on this. It's currently in a semi-working state which has been usable for my company. If someone with more Haskell experience than me wants to take this on, then by all means.

mpdifran · 2021-08-03T13:50:01Z

What's left to do to get this in? Would it make sense to get this in partially working, and open another PR to fix the remaining issues?

evandcoleman · 2021-08-03T14:07:27Z

@mpdifran See the comments above from @tmspzz. He indicated the changes that should be made.

tmspzz · 2021-10-14T12:17:56Z

Closing in favor of #247

tmspzz · 2021-10-14T12:18:47Z

thanks @evandcoleman for the amazing work! Now @vikrem will push it over the finish line. 🥳

add --use-xcframeworks flag

5fb46cc

hlintBot reviewed Apr 15, 2021

View reviewed changes

evandcoleman changed the title ~~Add Support for XCFrameworks~~ [WIP] Add Support for XCFrameworks Apr 15, 2021

tmspzz reviewed Apr 22, 2021

View reviewed changes

vikrem mentioned this pull request Oct 8, 2021

Add Support for XCFrameworks #247

Merged

tmspzz closed this Oct 14, 2021

Uh oh!

Conversation

evandcoleman commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Known Issues

Uh oh!

hlintBot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

hlintBot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

hlintBot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

hlintBot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

hlintBot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

hlintBot commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmspzz commented Apr 22, 2021

Uh oh!

tmspzz commented Apr 22, 2021

Uh oh!

tmspzz Apr 22, 2021

Choose a reason for hiding this comment

Uh oh!

evandcoleman commented Apr 25, 2021

Uh oh!

tmspzz commented Apr 26, 2021

Uh oh!

evandcoleman commented Apr 26, 2021

Uh oh!

tmspzz commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damiankolasinski commented May 17, 2021

Uh oh!

mpdifran commented Jun 25, 2021

Uh oh!

jangelsb commented Jul 7, 2021

Uh oh!

jesus-mg-ios commented Aug 2, 2021

Uh oh!

evandcoleman commented Aug 3, 2021

Uh oh!

mpdifran commented Aug 3, 2021

Uh oh!

evandcoleman commented Aug 3, 2021

Uh oh!

tmspzz commented Oct 14, 2021

Uh oh!

tmspzz commented Oct 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

evandcoleman commented Apr 15, 2021 •

edited

Loading

hlintBot commented Apr 15, 2021 •

edited

Loading

tmspzz commented Apr 28, 2021 •

edited

Loading