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.

Sign up for GitHub

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

Jump to bottom

Add | Local DB support for ARM64 and Common Code for LocalDB #2042

Closed
wants to merge 19 commits into from
Closed

Add | Local DB support for ARM64 and Common Code for LocalDB #2042

wants to merge 19 commits into from

Conversation

Kaur-Parminder
Copy link
Contributor

@Kaur-Parminder Kaur-Parminder commented May 18, 2023 •
edited
Loading

  • Added Matteo-T PoC for ARM64 Support. PoC for ARM64 support of LocalDB #1952 (addressed comments)
  • Moved LocalDB logic out from MangeSNI to common code base. (LocalDB code to Pre-SNI, changed SNI Error messages to throw exceptions rather, SNITraceLogs-->Tracelogs, moved DataSource.cs to common code)
  • Removed Netfx LocalDB logic.

@Kaur-Parminder Kaur-Parminder changed the title Local DB support for ARM64 and Code Merge Local DB support for ARM64 and Common Code for LocalDB May 18, 2023
David-Engel
David-Engel reviewed May 18, 2023
View reviewed changes
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs Outdated Show resolved Hide resolved
...Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalDB.Windows.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalDB.Windows.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalDB.Windows.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalDB.Windows.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalDB.Windows.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalDB.Windows.cs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 19, 2023 •
edited
Loading

Codecov Report

Patch coverage: 45.12% and project coverage change: -0.61% ⚠️

Comparison is base (ceaf8ec) 70.73% compared to head (9e561a0) 70.12%.
Report is 2 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2042      +/-   ##
==========================================
- Coverage   70.73%   70.12%   -0.61%     
==========================================
  Files         306      304       -2     
  Lines       61975    61875     -100     
==========================================
- Hits        43835    43389     -446     
- Misses      18140    18486     +346
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.46% <40.34%> (-0.03%) ⬇️
netfx 68.08% <23.19%> (-1.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...tcore/src/Microsoft/Data/SqlClient/LocalDB.Unix.cs 0.00% <0.00%> (ø)
...netcore/src/Microsoft/Data/SqlClient/LocalDBAPI.cs 88.88% <ø> (ø)
...core/src/Microsoft/Data/SqlClient/SNI/SNICommon.cs 55.12% <ø> (ø)
...tcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs 44.91% <ø> (-4.55%) ⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 80.16% <ø> (-0.02%) ⬇️
...c/Microsoft/Data/Interop/SNINativeMethodWrapper.cs 75.43% <ø> (ø)
...t/netfx/src/Microsoft/Data/SqlClient/LocalDBAPI.cs 29.62% <ø> (-3.04%) ⬇️
...ient/src/Microsoft/Data/Common/AdapterUtil.Unix.cs 50.00% <0.00%> (-50.00%) ⬇️
...nt/src/Microsoft/Data/ProviderBase/TimeoutTimer.cs 62.16% <0.00%> (-19.99%) ⬇️
...oft/Data/SqlClient/TdsParserSafeHandles.Windows.cs 93.00% <ø> (-0.07%) ⬇️
... and 9 more

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Kaur-Parminder Kaur-Parminder marked this pull request as ready for review May 26, 2023 19:21
@David-Engel David-Engel added this to the 5.2.0-preview3 milestone Jun 13, 2023
@JRahnama JRahnama changed the title Local DB support for ARM64 and Common Code for LocalDB Add | Local DB support for ARM64 and Common Code for LocalDB Jun 13, 2023
DavoudEshtehari
DavoudEshtehari reviewed Jun 27, 2023
View reviewed changes
Copy link
Member

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

Remove the unnecessary using in SNIProxy.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/DataSource.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/LocalDB.Unix.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/LocalDB.uap.cs Show resolved Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/LocalDBAPI.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/LocalDBAPI.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/LocalDBAPI.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/LocalDBAPI.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/LocalDBAPI.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/TimeoutTimer.cs Outdated Show resolved Hide resolved
DavoudEshtehari
DavoudEshtehari requested changes Jun 30, 2023
View reviewed changes
Copy link
Member

@DavoudEshtehari DavoudEshtehari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

An invalid data source on ARM machines should respect a relevant exception like x64.

LocalDb tests should be added to the pipeline and run on ARM agents as well.

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj Outdated Show resolved Hide resolved
@Kaur-Parminder
Merge branch 'main' into LocalDBSupportForARM64andCodeMerge
d91a7b6 
# Conflicts:
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs
#	src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
#	src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs
#	src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
@Kaur-Parminder
resolving conflict
6cd4900
@Kaur-Parminder
Add Platform not supported test
ba2dbf3
@Kaur-Parminder
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JRahnama JRahnama removed this from the 5.2.0-preview4 milestone Nov 21, 2023
@kf-gonzalez kf-gonzalez marked this pull request as draft January 2, 2024 19:43
@JRahnama JRahnama closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@David-Engel David-Engel David-Engel approved these changes

@JRahnama JRahnama Awaiting requested review from JRahnama

@arellegue arellegue Awaiting requested review from arellegue

@DavoudEshtehari DavoudEshtehari Awaiting requested review from DavoudEshtehari

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Kaur-Parminder @David-Engel @DavoudEshtehari @JRahnama