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

chore: Add support for Visual Studio 2019 #135

Merged
merged 4 commits into from Sep 3, 2019

Conversation

@ciband
Copy link
Collaborator

commented Aug 25, 2019

Updated CMakeSettings.json to support Visual Studio 2019 compilers and schema.

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactor
  • Performance
  • Tests
  • Build
  • Documentation
  • Code style update
  • Continuous Integration
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR release a new version?

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the develop branch, not the master branch
  • All tests are passing
  • New/updated tests are included

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)
chore: Add support for Visual Studio 2019
Updated CMakeSettings.json to support Visual Studio 2019 compilers and schema.
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Thanks for submitting this pull request! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@codecov-io

This comment has been minimized.

Copy link

commented Aug 25, 2019

Codecov Report

Merging #135 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #135   +/-   ##
========================================
  Coverage    91.82%   91.82%           
========================================
  Files           33       33           
  Lines          856      856           
========================================
  Hits           786      786           
  Misses          70       70

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93022a8...9f3421b. Read the comment docs.

@sleepdefic1t

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@ciband
Just to double check because I'm not familiar this,
will this still support 2017, or does this basically change the requirement to 2019?

@ciband

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 27, 2019

@sleepdefic1t

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@ciband
Is there a way to keep support configs for both, 2017 & 2019?
I'd prefer that route if possible.

@ciband

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 27, 2019

There is.

Just to be clear, are you wanting to support developers developing cpp-crypto in VS2017 and VS2019?

or

Support only VS2019 (latest/greatest) for developers and then support VS2017 and VS2019 in CI.

The pattern being develop in the latest IDE but we support the latest 2 version of the compiler.

We don't do this for Linux, we kind of do this for Mac with xcode.

@sleepdefic1t

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

@ciband
CI can be 2019.
I’d like to support 2017 & 2019 users.

I just wasn’t sure if having the 2019 config means 2017 users would have to do their own configuration or not.

@ciband

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2019

@sleepdefic1t I think we need to be consistent. If we want developers to be able to use the 2017 compiler or the 2019 compiler, then I think CI should reflect that (meaning once we actually get Windows CI up and running via Appveyor or CircleCI).

For this PR and its scope, I'm hearing to support both so I will Add the VS2017 configs back in.

@sleepdefic1t

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Okay, sounds good.

I know nothing about Windows Dev, I just know a lot of the students we saw at Hackathons didn’t usually have the latest releases. Lots of 2015 configs in repos too, so I don’t want to force people into 2019 if we can just offer it as an option.

I appreciate your guidance on the Windows stuff.
Thanks again, @ciband

ciband added 2 commits Aug 30, 2019
chore: Add VS2017 configurations
Added VS2017 configs and removed Linux configs for Windows.
@ciband

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

Appveyor failure due to plan time execution limits. Debug passed, release was aborted.

This works on VS2019 with the VS2017 changes. It tries to invoke the VS2017 compiler under VS2019 for the appropriate config.

I still need to test on a pure VS2017 machine to see how it handles the VS2019 configs but I don't foresee any issues.

My laptop HD is small so I had to uninstall VS2017.

@ciband

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

@sleepdefic1t VS2017 targets work under VS2017 IDE. The VS2019 targets to not work, which is expected. They give reasonable diagnostics as to why they do not work.

I think this PR is ready for review.

@sleepdefic1t sleepdefic1t self-requested a review Sep 2, 2019

@sleepdefic1t
Copy link
Contributor

left a comment

appveyor errors not due to these changes.

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

A contributor has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

A contributor has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@faustbrian faustbrian merged commit e4dac9a into ArkEcosystem:develop Sep 3, 2019

7 of 8 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: build-arduino-default Your tests passed on CircleCI!
Details
ci/circleci: build-linux-clang-5 Your tests passed on CircleCI!
Details
ci/circleci: build-linux-default Your tests passed on CircleCI!
Details
ci/circleci: build-linux-gcc7 Your tests passed on CircleCI!
Details
ci/circleci: build-macos-9-2 Your tests passed on CircleCI!
Details
ci/circleci: build-macos-9-3 Your tests passed on CircleCI!
Details
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Your pull request has been merged but was not assigned a bounty tier. A maintainer will assign a bounty tier to this pull request in the next few days.

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Your pull request has been merged and marked as tier 4. It will earn you $20 USD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.