-
Notifications
You must be signed in to change notification settings - Fork 0
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
[WIP] Add Support for PodBuilder #1
Conversation
82dbe51
to
df75f1d
Compare
@blender I implemented the parser for
Unfortunately I found out that I also need to parse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just parsed lines starting with
pod
(and that end with# pb<...>
)
Sounds reasonable
In the
Podfile.restore
, subspec references likeAFNetworking/NSURLSession
can occur, but PodBuilder will only build a single framework.
I think you should treat this independently of what Rome does. So if you need to track subspecs for the sake of correctly parsing/processing the Podbuilder information do so. Then for Rome you can construct something else that collapses (or filters out) the duplicate information accordingly.
I have not filtered out duplicates across targets or subspecs, I guess this can be done at a later point when the data from the parsed
Podfile.restore
andPodBuilder.podspec
are combined?
The parser should just do the parsing. Deal with duplicates later.
I could parse the line starting with
platform
fromPodfile.restore
, but I was not sure how to parse it properly. Any hints?
I'm eyeballing it so don't assume this works
Parsec.spaces
>> Parsec.string "platform" -- parses "platform"
>> Parsec.spaces
*> Parsec.between (Parsec.symbol ":") (Parsec.symbol ",") -- parses " :whateverstring, "
<* Parsec.spaces
>> Parsec.between (Parsec.symbol "'") (Parsec.symbol "'") -- parses " 'whateverstring' "
Do the data structures
PodfileEntry
,PodName
andPodVersion
make sense like this for further processing?
They look good to me, if you want to parse the platform add PodPlatform
Should I write some unit tests? Due to
Parsec.try parseMaybePodfileEntry
, parsing errors are silently swallowed, which makes the parser a little fragile. Should I add a stripped downPodfile.restore
toTests/Fixtures/
, and write an HSpec test?
Yes, add as many as fixtures as you want, they don't have to be stripped down. Having the real deal is better. I don't recall how I do the error reporting for the Cartfile but I guess that it should be the same.
{-# LANGUAGE NamedFieldPuns #-} | ||
{-# LANGUAGE RecordWildCards #-} | ||
|
||
module Data.Carthage.Cartfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this module name seems off
@blender Thanks! But change of plans: I talked to @tcamin (maintainer of PodBuilder, actually he works in a sister company), and we found out that even when parsing
So I will parse this output instead of So parsing this should be easy, but getting it into blender/Rome not, since it's not stored in a file. I thought about STDIN, what do you think? I'd like to avoid temporary files. |
I think your best option is first going via file. Once successful we can think of how to use stdin. |
df75f1d
to
33d3781
Compare
@blender now that was easy, parsing of the json is done. My next steps would be to add a configuration option to the |
Ok first of a few questions:
I would start by adding a CLI option with default value
(Maybe there is a more appropriate name for the flag) you can then analyzer the CLI flag right in the The function to upload and download and upload to S3 are pretty generic and agnostic of Carthage (or so I recall). The main trick is a few HashMap that store: repository name -> binary name I would ignore any networking to start with and just do the local filesystem caching for now. |
import Data.Aeson | ||
import qualified Data.ByteString.Lazy as B | ||
|
||
type PodBuilderInfo = Map String PodInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be Data.Map.Strict
parsePodBuilderInfoByteString :: B.ByteString -> Maybe PodBuilderInfo | ||
parsePodBuilderInfoByteString = decode | ||
|
||
parsePodBuilderInfo :: IO (Maybe PodBuilderInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MonadIO m => m (Maybe PodBuilderInfo)
|
||
parsePodBuilderInfo :: IO (Maybe PodBuilderInfo) | ||
parsePodBuilderInfo = | ||
parsePodBuilderInfoByteString <$> B.readFile podBuilderInfoFileName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a withFile
that will close the descriptor. I might be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it should be ok:
Because we don't get a handle with which to identify our file, we can't close it manually, so Haskell does that for us when we use
readFile
.
http://learnyouahaskell.com/input-and-output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad
I think I need neither. The I'm not sure about the
Ok done. I also added a
Ok, next up :-) |
I think you still need the All is nice till a framework has a .modulemap for a system library and the binaries are not relocatable. |
mmm, can you show me the code? I'm not sure what the purpose of this would be since you can populate |
Also you might want to pull from my repo. I changed a few things and now the build is much faster. |
Hm good hint, do you have an example so I can try with PodBuilder?
Not every framework is a Swift framework, so not all need the common cache prefix. PodBuilder provides the swift_version only for Swift frameworks, see Line 60 in 6f1ca1d
|
Non-relocatable framework example: https://github.com/daltoniam/Starscream
Sure. So you plan to upload/download each framework from the appropriate location in the Cache based on this information? I see how this can be valuable to reduce duplication in the cache but I feel like it's a bit of an overkill and also makes the current cache structure incompatible between Carthage/Podbuilder. It would be better in my opinion not to alter the cache structures. Why? Because in the end it should not matter if the uploader was using Carthage or Podbuilder |
Exactly, I‘d put it into the filename additionally to the Just hypothetically if I used the cache prefix instead, how would I in Rome check if the local frameworks need to be replaced by ones of the cache (because we don‘t know which Swift version The local frameworks have, and which would be expected)?
While a good idea in theory, this won‘t work in practice anyways, even when I would ignore the swift_version. Cocoapods supports subspecs, that means a built framework would only include one of several different sets of subspecs. The cached framework should only be used for exactly the same set of expected subspecs. Therefore I‘d also put the subspec names (or a hash of the array of subspec names) into the filename. |
Rome doesn't know things need to be replaced. It's the responsibility of the user to enter the right prefix (maybe by invoking rome like Rome is just a vector (a transport system) to the cache. Rome knows where to get and where to put binaries at, but that's all. It's pretty dumb. I could make it smarter but I don't know if I want to take the freedom away from the user and put the burden on the tool on when and where do things. The more I automate the more error prone the process is. More specifically: Carthage produces a They are upload at a path constructed in the following way:
When the user runs Alone Rome won't do anything for you. However you can combine command to make it smarted (Functional programming strikes again in the design :D), for example:
to upload only what is not already in the caches. In theory I could read the .version files to know if a framework is objective-c, what version of the compiler it was built with and so on... but:
I'm not sure I understand why. Maybe you can help me? A binary is a binary and to Carthage (at least) all that it matters is that it's placed in the right directory. I imagine Podbuilder also expects binaries to be at a certain location on disk so it's just a matter of placing them there on the user's machine. I'm not sure why where they are in the cache would matter.
That would also break the compatibility of the caches. |
Hm, maybe I missed a piece: until now I thought
Good point, also in the case of PodBuilder I currently do not have the information a priori whether it will be a Swift framework or not. Hm.
The same framework name could be produced by completely different source code, because of subspecs. For example see https://github.com/AFNetworking/AFNetworking/blob/master/AFNetworking.podspec. When I build AFNetworking just with subspec
I would not change how the filenames were constructed for in Carthage mode of course. |
Yes. In theory I could limit what it downloads by looking at the .versions files if any. That's actually a good point. On the first run the user doesn't have any, so download everything. On subsequent runs check what versions files are out of sync with the truth (the Cartfile.resolved). I never thought of this. Thanks for the inspiration! 😄
so a subspec is a portion of a complete framework built on demand? If this is the case then I understand why the caches binaries in the cache can't be shared. |
Molto volentieri! BUT: this won‘t work in the case when using the cache prefix for the Swift version, because you have no way to verify if the local framework stems from the cache with the correct prefix. That‘s why I thought I need to give more information to the dumb vector.
Yes, with the exception that there might even be mutually exclusive subspecs. E.g. https://github.com/at-internet/atinternet-apple-sdk/blob/master/ATInternet-Apple-SDK.podspec contains separate subspecs for the main target and the app extension. In this case, PodBuilder even builds two differently postfixed frameworks. See https://github.com/Subito-it/PodBuilder/blob/master/README.md#subspecs_to_split for more information. |
6f1ca1d
to
f4a44c4
Compare
I'm not sure why I would need to verify the Swift version. Rome doesn't know about Swift versions to begin with. The Swift version handling is artificial and manual based on the prefix which is user input. Nothing presents having a cache prefix like What it can do however is check what it needs to download based on cross referencing the hashes in the Cartfile.resolved and in the |
Instead of branching, I tried to introduce a sum type
Maybe I'm somehow wrong, but consider this example: Scenario 1:
Scenario 2:
I currently can't really think of a way to give Rome this information in a more abstract way than to teach it about the Swift version. Sure, I could give up on the optimization of not downloading everything, but where would the fun be in that? 😄 |
Oh I see. Sure scenario 2 won't work, however I don't know how common this is. It seems like an overkill to me to worry about the same commit building for 2 different versions of Swift and people deciding to pull in one from an older version of the compiler. If the user wants to do so I think it should be his responsibility non to run the optimized download/upload. The current download/upload is not optimized anyways so that would have to be another CLI option like:
|
Seems good. Great work! 🎉 |
bf5cf16
to
5e3745f
Compare
Ok, slowly getting there. The generated paths are not yet correct, but now I have the |
…to compare it to the swift_version of pod_builder info output" since the swift compiler version is now encoded in the PodBuilderInfo.json
…e they do not exist in these projects and are not needed
…separate paths for Carthage and PodBuilder, and use it instead of the old carthage-specific function everywhere
…_remoteFrameworkPath to it in order to encapsulate framework path generation in it currently not setting it differently dependent on BuildTypeSpecificConfiguration, but that will follow
…the upload command - yet need to add path function fields to FrameworkVector though
…the download command - yet need to add path function fields to FrameworkVector though
…ion _remoteFrameworkPath anyways
…fix not being applied to temp_remoteDsymPath
…ich return Maybes instead of checking for buildTypeConfig in Lib - implemented for uploading
a707b84
to
e5216c4
Compare
rebased everything on the latest master, phew, this was quite some work! 😅 |
…rkVectorPaths, so the FrameworkVector can be decomposed more easily when adding more path fields without the need to touch too much unrelated code
Ok, moved all the |
…and renamed some functions for safety too
Since SwiftPM is now integrated into Xcode 11, I will stop developing this PR. Also, the implementation strategy conflicts massively with tmspzz#148, and a lot of work would need to be put in to resolve this issue, and additionally the issue of supporting both device and simulator builds. |
oh damn! Too bad :( I hope you had a nice time playing around with Haskell though! |
As discussed in tmspzz#155, support for PodBuilder would be nice additionally to Carthage.
Parse and analyze the Podfile.restore file to find out which framework versions the project depends onParse and analyze the PodBuilder.podspec file to find out which frameworks would need to be generatedParse the PodBuilder.plist files and use them instead of Carthage version filespod_builder info
json output from a filepodbuilder info
output via STDIN or as a parameter (a temporary file would not be so nice)pod_builder info
contains theswift_version
for each built Swift framework, and we need to compare it to the expected one, and use it as part of the cache key