Skip to content

[build-script] Improvements to CMakeOptions. #23303

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

Merged
merged 1 commit into from
Jun 12, 2019

Conversation

drodriguez
Copy link
Contributor

Applies to SR-237

This code was extracted and improved from #23038.

CMakeOptions was briefly used in the cmake module, but mostly unused in
the rest of the modules, however, several products were dealing with
what eventually will end as CMake options, but not using the already
existing code, and duplicating it.

The changes tries to unify all usage around CMakeOptions which avoids
code duplication. Additionally, it provides some more API surface in
CMakeOptions to make it more useful.

  • The initializer can be passed a list of tuples or another CMakeOptions
    to copy into the newly created options.
  • Boolean Python values are treated automatically as boolean CMake
    options and transformed into TRUE and FALSE automatically.
  • Provides extend, which will add all the tuples from a list or all
    the options from another CMakeOptions into the receiving
    CMakeOptions.
  • Provides __contains__, which makes checking for existing options a
    little easier (however, being CMakeOptions basically a list, the
    checking has to include also the value, which is not that helpful).
  • Modify LLVM and Swift products to use CMakeOptions. It makes the
    boolean values clearer and avoid a lot of repetitive string
    formatting.
  • Modify the tests to make them pass and provide new test for newly
    introduced features.

/cc @Rostepher, @compnerd

@compnerd
Copy link
Member

This looks good to me, but, I think that I would defer to @Rostepher or @ddunbar here.

@drodriguez
Copy link
Contributor Author

Rebased on top of current master.

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - ce2463d60ccbfd8298c941bb7cdcf82184f18ea4

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - ce2463d60ccbfd8298c941bb7cdcf82184f18ea4

@drodriguez drodriguez force-pushed the cmake-options-improvements branch from ce2463d to 9ef97f0 Compare April 29, 2019 22:43
@drodriguez
Copy link
Contributor Author

Fix a problem where the version parameters are neither bools, numbers or strings (they get parsed into specific types by the argparser), and cannot be passed directly to CMakeOptions. I tried to fix the tests (which actually tries to test this with naked strings), but importing across modules (swift_build_support and build_swift) seems very complicated.

(The Linux problem seemed unrelated, though)

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - ce2463d60ccbfd8298c941bb7cdcf82184f18ea4

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - ce2463d60ccbfd8298c941bb7cdcf82184f18ea4

@drodriguez drodriguez force-pushed the cmake-options-improvements branch from 9ef97f0 to 632f936 Compare April 30, 2019 18:10
@drodriguez
Copy link
Contributor Author

Making the linter happy.

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 9ef97f0949bc7d9bbf22434140e570b694023374

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 9ef97f0949bc7d9bbf22434140e570b694023374

@drodriguez drodriguez force-pushed the cmake-options-improvements branch from 632f936 to 1170c57 Compare April 30, 2019 23:33
@drodriguez
Copy link
Contributor Author

Recovering the changes overwritten in the last forced push.

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 632f936ef8eede3b21f50ce48b3f7b0de81d6a74

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 632f936ef8eede3b21f50ce48b3f7b0de81d6a74

CMakeOptions was briefly used in the cmake module, but mostly unused in
the rest of the modules, however, several products were dealing with
what eventually will end as CMake options, but not using the already
existing code, and duplicating it.

The changes tries to unify all usage around CMakeOptions which avoids
code duplication. Additionally, it provides some more API surface in
CMakeOptions to make it more useful.

- The initializer can be passed a list of tuples or another CMakeOptions
  to copy into the newly created options.
- Boolean Python values are treated automatically as boolean CMake
  options and transformed into `TRUE` and `FALSE` automatically.
- Provides `extend`, which will add all the tuples from a list or all
  the options from another `CMakeOptions` into the receiving
  `CMakeOptions`.
- Provides `__contains__`, which makes checking for existing options a
  little easier (however, being `CMakeOptions` basically a list, the
  checking has to include also the value, which is not that helpful).
- Modify LLVM and Swift products to use `CMakeOptions`. It makes the
  boolean values clearer and avoid a lot of repetitive string
  formatting.
- Modify the tests to make them pass and provide new test for newly
  introduced features.
@drodriguez drodriguez force-pushed the cmake-options-improvements branch from 1170c57 to 6bf0826 Compare May 14, 2019 20:43
@drodriguez
Copy link
Contributor Author

Ready for review. Fixed the conflicts with current master, and checked it still works as intended.

@swift-ci please test

@drodriguez
Copy link
Contributor Author

@Rostepher: I think this would be valuable going forward, and might simplify the later changes. If there's some feedback (positive or negative) I can also work on it. If we don’t want the changes at all, it would also be nice to know, to try to work around them, and not be hold by them. Thanks a lot.

Copy link
Contributor

@Rostepher Rostepher left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

@drodriguez drodriguez merged commit 5a2fd74 into swiftlang:master Jun 12, 2019
@drodriguez drodriguez deleted the cmake-options-improvements branch June 12, 2019 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants