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

API changes tracking #2673

Closed
wants to merge 20 commits into from
Closed

API changes tracking #2673

wants to merge 20 commits into from

Conversation

Udumft
Copy link
Contributor

@Udumft Udumft commented Oct 2, 2020

Adding a script to resolve tracking Public API changes using CI script.

Currently at MVP stage and there are some known upgrades incoming:

  • storing the APIDiffReport separately and checking out it during the build
  • caching and restoring Base API log to avoid building it every time
  • parameterizing Diff utility to control which changes exactly we are interested, which are blocking, which are not
  • need a way to specify Base version for comparison

@Udumft Udumft added build Issues related to builds and dependency management. feature New feature request. wip labels Oct 2, 2020
@Udumft Udumft added this to the v1.1.0 milestone Oct 2, 2020
@Udumft Udumft requested a review from a team October 2, 2020 11:31
@Udumft Udumft self-assigned this Oct 2, 2020
Comment on lines +345 to +357
- *prepare-mapbox-file
- *prepare-netrc-file
- *update-carthage-version
- *restore-cache
- *install-dependencies
- *install-dependencies-12
- run:
name: Install prerequisites
command: if [ $(xcversion simulators | grep -cF "iOS << parameters.iOS >> Simulator (installed)") -eq 0 ]; then xcversion simulators --install="iOS << parameters.iOS >>" || true; fi
- *save-cache
- save-api-diff-cache-by-tag:
api_diff_cache: << parameters.base_api_tag >>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These steps should be run only if cache above was not restored. I could not find a working solution, so any suggestions are highly welcome.

Comment on lines 365 to 371
- run:
name: Generating MapboxCoreNavigation API Diff
command: cd scripts/APIDiffReport && swift run APIDiffReport diff $CIRCLE_WORKING_DIRECTORY/original_api/core_navigation_log.json $CIRCLE_WORKING_DIRECTORY/api_logs/core_navigation_log.json
- run:
name: Generating MapboxNavigation API Diff
command: cd scripts/APIDiffReport && swift run APIDiffReport diff $CIRCLE_WORKING_DIRECTORY/original_api/navigation_log.json $CIRCLE_WORKING_DIRECTORY/api_logs/navigation_log.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These basically log changes to output and fail the CI. Should we Report the diff some other way?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now. When we work on a major version, we can make the build bot optional.

Comment on lines 70 to 73
let ignoredKeys = Set(arrayLiteral: "key.doc.line", "key.parsed_scope.end", "key.parsed_scope.start", "key.doc.column", "key.doc.comment", "key.bodyoffset", "key.nameoffset", "key.doc.full_as_xml", "key.offset", "key.fully_annotated_decl", "key.length", "key.bodylength", "key.namelength", "key.annotated_decl", "key.doc.parameters", "key.elements", "key.related_decls",
"key.filepath", "key.attributes",
"key.parsed_declaration",
"key.docoffset", "key.attributes")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we can configure which type of changes exactly we are interested right now.

Comment on lines 285 to 286
if accessibility as! String != "source.lang.swift.accessibility.public" &&
accessibility as! String != "source.lang.swift.accessibility.open" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There we control accessibility level of interest. Also, we can filter out undocumented or explicit ':nodoc:' items.

@1ec5 1ec5 changed the base branch from master to main October 7, 2020 18:20
@Udumft
Copy link
Contributor Author

Udumft commented Oct 27, 2020

A sample diff output with v.0.40 could be found here.

@kshehadeh, it would be also nice to receive any feedback or a use-cases from the Directions team so we could adjust the tool.

@kshehadeh
Copy link

A sample diff output with v.0.40 could be found here.

@kshehadeh, it would be also nice to receive any feedback or a use-cases from the Directions team so we could adjust the tool.

This is very promising. I don't yet have access to CircleCI, but I'll check it out as soon as I do.

@Udumft
Copy link
Contributor Author

Udumft commented Oct 29, 2020

Here's one as attachment:
log.txt

@kshehadeh
Copy link

I'm having some trouble reading the output. Could you help explain what we're looking at there?

@Udumft
Copy link
Contributor Author

Udumft commented Nov 2, 2020

The output lists detected updated made to public symbols, which are considered as breaking change. It should detect code-related points mentioned in this comment. Log contains the symbol name and it's location

@1ec5 1ec5 self-assigned this Nov 2, 2020
@Udumft Udumft force-pushed the vk/399-api-stability branch from 0717b1b to 636a931 Compare November 13, 2020 13:18
@1ec5 1ec5 modified the milestones: v1.1.0, v1.2.0 Nov 13, 2020
@Udumft
Copy link
Contributor Author

Udumft commented Nov 17, 2020

I've added some options to control the diff generation process from the command line. Now you can choose symbol access level you are interested in or if only documented symbols are included (useful for cases when only documented entities are considered publicly supported).

For example, for added section of code like this:

public var newUndocumentedProperty = false
    
    /// There is a valid doc on this property
    public var newDocumentedProperty = true
    
    /// :nodoc:
    public var newNODOCProperty = false
    
    internal var newInternalVar = false
    
    /// Another valid doc here
    internal var newInternalDocumentedVar = false

Only newDocumentedProperty property will be detected as a braking change:

**** BREAKING CHANGES DETECTED ****

Breaking changes in 'NavigationViewController'
new var: newDocumentedProperty in NavigationViewController
Program ended with exit code: 1

I would also like to provide ability for user to select exact items to check (like only detect new functions, or only check structs and so on), but it is unclear how to provide such description. Such points are controlled by filtering api log JSON keys, and keys have form like key.parsed_declaration. To control list of allowed keys it would be handy to pass in some config file, but that seems like out of scope for current task. Or at least I should not design it with knowing some real use cases for that.

@1ec5
Copy link
Contributor

1ec5 commented Dec 18, 2020

I would also like to provide ability for user to select exact items to check (like only detect new functions, or only check structs and so on), but it is unclear how to provide such description. Such points are controlled by filtering api log JSON keys, and keys have form like key.parsed_declaration.

This would be an interesting capability to have, but it could be tail work as long as the default is reasonable. To avoid a false sense of security, I think we’d want the default to check anything that could be a backwards-compatible change, at the risk of overcommunicating. We can make the check non-required at first.

To control list of allowed keys it would be handy to pass in some config file, but that seems like out of scope for current task. Or at least I should not design it with knowing some real use cases for that.

Yeah, I don’t have a concrete use case in mind for filtering. Perhaps with the churn that’ll come with v2.0, we may want to sort changes by type or severity, but I don’t see us sometimes caring about method names and sometimes not caring.

@1ec5
Copy link
Contributor

1ec5 commented Dec 18, 2020

In case it helps, here’s a list I wrote up last year with help from @JThramer, before we dropped Objective-C support in #2230, of changes that would be considered backwards-incompatible:

  • Upgrading a dependency to a new major version (?)
  • Migrating to a new major version of Swift or Xcode
    Mitigation: Conditionally supporting the older version
  • Increasing the minimum deployment target
  • Renaming a public, documented type or constant (or its runtime name)
    Mitigation: Leaving the old type name as a type alias or @compatibility_alias
  • Renaming a public, documented method or function (or its Objective-C selector name)
    Mitigation: Leaving a deprecated stub under the old name (does renamed: count?)
  • Removing a public, documented type, method, function, constant, or enumeration case
  • Adding a public enumeration case (?)
  • Adding a field to a public, documented C struct
  • adding a required method to a protocol
  • Changing the optionality (nullability) of a public, documented property, or an argument or return value of a public, documented method or function
  • Adding an argument to a method or function
    • Giving the argument a default value (not in Objective-C)
  • Adding an argument to a case of an associated-value enumeration
  • Changing the default value of a public, documented property, constant, or argument (?)

@zugaldia
Copy link
Member

@Udumft I think the CLI that @kshehadeh was interested in is the one that landed with mapbox/mapbox-directions-swift#469 (to validate Directions API responses) as opposed to this one (which is used to validate the SDK API contract).

@S2Ler S2Ler assigned S2Ler and unassigned dersim-davaod May 27, 2021
@S2Ler
Copy link
Contributor

S2Ler commented Jun 22, 2021

Closing in favour of #3103

@S2Ler S2Ler closed this Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to builds and dependency management. feature New feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants