jamesmontemagno / vsts-mobile-tasks

VSTS Tasks for Mobile!
MIT License
79 stars 22 forks source link

iOS Bump Version is overwriting CFBundleShortVersionString #26

Open chkpnt opened 4 years ago

chkpnt commented 4 years ago

When ios-bump-version is used in a classic build pipeline, the task's behaviour regarding versionName corresponds to the description ("leave blank to use existing value in info.plist"): CFBundleShortVersionString isn't overwritten.

But when the same task is used in a yaml-pipeline, CFBundleShortVersionString is overwritten by 1.0.$(Build.BuildId), which is the defaultValue from task.json.

- task: ios-bundle-version@1
  displayName: 'Bump iOS Versions in path/to/Info.plist'
  inputs:
    sourcePath: path/to/Info.plist
    versionCode: '$(Build.BuildNumber)'
chkpnt commented 4 years ago

Okay, I have to specify versionName explicitly:

- task: ios-bundle-version@1
  displayName: 'Bump iOS Versions in path/to/Info.plist'
  inputs:
    sourcePath: path/to/Info.plist
    versionCode: '$(Build.BuildNumber)'
    versionName: 

The yaml in my previous example have been generated by the yaml-export in the classic build pipleline, but obviously it doesn't behave in the same way. :-(

In my opinion, an empty versionName in the yaml is quite confusing. Therefore I suggest introducing a boolean like updateVersionName or a special versionName: keep to keep the yaml more readable.

SkyeHoefling commented 4 years ago

Can you include an example Info.plist, I think that will help in determining what the problem is. I just took a look at the code and if the versionName isn't specified it shouldn't do anything with it.

https://github.com/jamesmontemagno/vsts-mobile-tasks/blob/c70ca69437909fb8ac919234fb00c3e52b2b82fb/tasks/iOSBumpVersion/task.ts#L74-L81

Having an example Info.plist will be useful to reproduce the problem to make an appropriate fix. Otherwise I'll just be taking educated guesses

chkpnt commented 4 years ago

Yeah, but versionName doesn't seem to be null or undefined anymore at this line, if a yaml-pipeline is used. It seems that Azure is replacing it with the defaultValue from task.json, which makes sense, of course, because that's what a default value is used to be. ;-)

I think the main issue I had was that I were confused by the fact I have to specify versionName explicitly. Maybe it's Azure's yaml-exporter, which could include the empty fields in the result.... But on the other hand, if you had a huge task with many fields left empty in the classic interface, the yaml would be huge....

I still think the yaml would be more readable if it contained a boolean or a special value as suggested in my second comment. But from my perspective, you can also close this issue.

I do not have any special in my Info.plist:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>CFBundleDevelopmentRegion</key>
    <string>$(DEVELOPMENT_LANGUAGE)</string>
    <key>CFBundleDisplayName</key>
    <string>DemoApp</string>
    <key>CFBundleExecutable</key>
    <string>$(EXECUTABLE_NAME)</string>
    <key>CFBundleIdentifier</key>
    <string>$(PRODUCT_BUNDLE_IDENTIFIER)</string>
    <key>CFBundleInfoDictionaryVersion</key>
    <string>6.0</string>
    <key>CFBundleName</key>
    <string>$(PRODUCT_NAME)</string>
    <key>CFBundlePackageType</key>
    <string>APPL</string>
    <key>CFBundleShortVersionString</key>
    <string>$(MARKETING_VERSION)</string>
    <key>CFBundleVersion</key>
    <string>1</string>
    <key>LSRequiresIPhoneOS</key>
    <true/>
    <key>UILaunchStoryboardName</key>
    <string>LaunchScreen</string>
    <key>UIRequiredDeviceCapabilities</key>
    <array>
        <string>armv7</string>
    </array>
    <key>UISupportedInterfaceOrientations</key>
    <array>
        <string>UIInterfaceOrientationPortrait</string>
        <string>UIInterfaceOrientationLandscapeLeft</string>
        <string>UIInterfaceOrientationLandscapeRight</string>
    </array>
    <key>UISupportedInterfaceOrientations~ipad</key>
    <array>
        <string>UIInterfaceOrientationPortrait</string>
        <string>UIInterfaceOrientationPortraitUpsideDown</string>
        <string>UIInterfaceOrientationLandscapeLeft</string>
        <string>UIInterfaceOrientationLandscapeRight</string>
    </array>
</dict>
</plist>
SkyeHoefling commented 4 years ago

You are absolutely right, I am thinking it may be best to update the versionName to have a default value of an empty string. Here is the code in question https://github.com/jamesmontemagno/vsts-mobile-tasks/blob/c70ca69437909fb8ac919234fb00c3e52b2b82fb/tasks/iOSBumpVersion/task.json#L66-L73

The only problem with creating a default value of an empty string is this could be a breaking change across other integrations that expect it to be defaulted to "1.0.$(Build.BuildId)".

With all of this being said, I think we can safely bump the Major Version of this task to make this fix. That will prevent any breaking changes for people using v1.

Example Integration

- task: ios-bundle-version@2
  displayName: 'Bump iOS Versions in path/to/Info.plist'
  inputs:
    sourcePath: path/to/Info.plist
    versionCode: '$(Build.BuildNumber)'

Note that the task name is now ios-bundle-version@2 which will use the new version.

SkyeHoefling commented 4 years ago

The defaultValue is very useful in the Visual Editor because it gives the user an idea of what the versionName should be. If we do set the default value to an empty string, we should move the "1.0.$(Build.BuildId)" as an example to the helpMarkDown

chkpnt commented 4 years ago

Yes, that sounds to be a good idea!

jamesmontemagno commented 4 years ago

makes sense previously there was only the visual part :) open to PRS :)

SkyeHoefling commented 4 years ago

I need to look at the docs, but I think this means we need to support both a v1 and a v2 implementation side-by-side. If you don't do that how would a new installation know where to get the v1 code vs the v2code. At least that is my working theory, only the docs have the true answer.

I'll do some research and submit a PR when it is ready for review.