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

Integrate abstractions projects into client build. #41

Conversation

MaxKsyunz
Copy link

  • Added projects under abstractions\src to OpenSearch.sln.
  • Removed build scripts specific to abstractions -- they are not needed any more.

Signed-off-by: MaxKsyunz maxk@bitquilltech.com

@MaxKsyunz MaxKsyunz requested review from Yury-Fridlyand and removed request for Yury-Fridlyand September 2, 2022 19:31
<SignAssembly>true</SignAssembly>
<AssemblyOriginatorKeyFile>$(SolutionDir)\build\keys\keypair.snk</AssemblyOriginatorKeyFile>
<AssemblyOriginatorKeyFile>$(MSBuildThisFileDirectory)\build\keys\keypair.snk</AssemblyOriginatorKeyFile>
Copy link
Author

@MaxKsyunz MaxKsyunz Sep 2, 2022

Choose a reason for hiding this comment

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

$(SolutionDir) is only defined inside an IDE, $(MSBuildThisFileDirectory) is defined by msbuild.

Relative path would have worked as well but may look ambiguous.

@Yury-Fridlyand
Copy link

Can you add misc files to the solution as well:

abstractions/*
abstractions/src/Directory.Build.props

@MaxKsyunz MaxKsyunz force-pushed the dev-os-2.0-integ-local-abstractions-no-build branch from 6663010 to 622321a Compare September 2, 2022 19:50
@@ -12,10 +12,10 @@ EndProject
Project("{F2A71F9B-5D33-465A-A702-920D77279786}") = "scripts", "build\scripts\scripts.fsproj", "{D6997ADC-E933-418E-831C-DE1A78897493}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "tests", "tests", "{6C4A2627-AF22-4388-9DF7-7A9AEACFD635}"
ProjectSection(SolutionItems) = preProject

Choose a reason for hiding this comment

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

What's the purpose to these whitespace changes in this file?

Copy link
Author

Choose a reason for hiding this comment

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

These were done by dotnet cli.

It looks like ProjectSection is an element under Project and so needs an indent.

ProjectSection(SolutionItems) = preProject
tests\Directory.Build.props = tests\Directory.Build.props
tests\.runsettings = tests\.runsettings
EndProjectSection

Choose a reason for hiding this comment

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

are the new intents needed?

Copy link
Author

Choose a reason for hiding this comment

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

These were done by the dotnet cli.

It looks like ProjectSection is an element under Project and so needs an indent.

@@ -49,29 +44,3 @@ Supports:
2. Released versions
* `MAJOR.MINOR.PATH` where `MAJOR` is still supported as defined by the EOL policy of OpenSearch.
* Note if the version exists but is not yet released it will resolve as a build candidate

Choose a reason for hiding this comment

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

Curious why this is removed from the readme?

Copy link
Author

Choose a reason for hiding this comment

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

It was asked for in the upstream PR.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@MaxKsyunz MaxKsyunz force-pushed the dev-os-2.0-integ-local-abstractions-no-build branch from 622321a to 4488a70 Compare September 2, 2022 21:47
@MaxKsyunz
Copy link
Author

@Yury-Fridlyand, as per your comment, I added abstractions\README.md and removed Directory.Build.props because it was functionally empty.

@Yury-Fridlyand Yury-Fridlyand merged commit 98fe5f9 into dev-os-2.0-integ-local-abstractions Sep 3, 2022
@Yury-Fridlyand Yury-Fridlyand deleted the dev-os-2.0-integ-local-abstractions-no-build branch September 3, 2022 01:59
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