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

[Win] Move references to the XDK to internal #1342

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

TyHolc
Copy link
Contributor

@TyHolc TyHolc commented Aug 21, 2023

Make references to the XDK internal-only to allow the external version to build properly.

b/296290769

Change-Id: I3bf4c59de1fd3163028d713586294a2a93f403be

@TyHolc TyHolc self-assigned this Aug 21, 2023
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #1342 (6b851e9) into main (3483cb4) will decrease coverage by 0.19%.
The diff coverage is n/a.

❗ Current head 6b851e9 differs from pull request most recent head 91b0c0f. Consider uploading reports for the commit 91b0c0f to get more accurate results

@@            Coverage Diff             @@
##             main    #1342      +/-   ##
==========================================
- Coverage   57.78%   57.59%   -0.19%     
==========================================
  Files        1915     1905      -10     
  Lines       95108    94759     -349     
==========================================
- Hits        54954    54575     -379     
- Misses      40154    40184      +30     

see 59 files with indirect coverage changes

@isarkis
Copy link
Member

isarkis commented Sep 1, 2023

@TyHolc, is this PR ready to be tested on XB1 runners in cobalt_sandbox?

@andrewsavage1
Copy link
Contributor

Is this ready to be submitted or do we have more testing to do for it?

@TyHolc
Copy link
Contributor Author

TyHolc commented Sep 18, 2023

Is this ready to be submitted or do we have more testing to do for it?

This should be ready, the try-builds passed (though xb1 tests have been failing for other reasons). Since it's kind of stale I'll rebase and rerun try-builds.

@andrewsavage1
Copy link
Contributor

This should be ready, the try-builds passed (though xb1 tests have been failing for other reasons). Since it's kind of stale I'll rebase and rerun try-builds.

Does it work on github?

@TyHolc
Copy link
Contributor Author

TyHolc commented Sep 18, 2023

Does it work on github?

@isarkis have you been able to rebase on top of this and try to build it via github?

@TyHolc
Copy link
Contributor Author

TyHolc commented Sep 18, 2023

@TyHolc, is this PR ready to be tested on XB1 runners in cobalt_sandbox?

Sorry I missed this, that was the first day I was sick. Yes, we can try this on the cobalt sandbox runners. Let me know if there's more that needs to be done.

@isarkis
Copy link
Member

isarkis commented Sep 19, 2023

@TyHolc, is this PR ready to be tested on XB1 runners in cobalt_sandbox?

Sorry I missed this, that was the first day I was sick. Yes, we can try this on the cobalt sandbox runners. Let me know if there's more that needs to be done.

Getting an error, see https://github.com/youtube/cobalt_sandbox/actions/runs/6240395072/job/16940433339

Can you take a look at youtube/cobalt_sandbox#513, do the Dockerfiles look correct?

@andrewsavage1
Copy link
Contributor

@TyHolc could you please try to build xb1 in docker locally?

Make references to the XDK internal-only to allow the external version
to build properly.

b/296290769

Change-Id: I3bf4c59de1fd3163028d713586294a2a93f403be
@TyHolc
Copy link
Contributor Author

TyHolc commented Oct 26, 2023

@TyHolc, is this PR ready to be tested on XB1 runners in cobalt_sandbox?

Sorry I missed this, that was the first day I was sick. Yes, we can try this on the cobalt sandbox runners. Let me know if there's more that needs to be done.

Getting an error, see youtube/cobalt_sandbox/actions/runs/6240395072/job/16940433339

Can you take a look at youtube/cobalt_sandbox#513, do the Dockerfiles look correct?

The Dockerfiles look correct. I verified the Win SDK link and win SDK version are correct.

As for that error, the missing classes should exist in the Win SDK and not the XDK, but maybe we have to pull in additional dependencies to cover ones lost by removing the XDK. Locally this build worked for me without needing to include anything additional (and while renaming all XDK folders in case the build was picking them up). I wasn't able to verify with Docker due to issues with my Windows machine though. I'll see if I can get that working again.

@TyHolc TyHolc merged commit e7bcf65 into youtube:main Oct 26, 2023
347 of 348 checks passed
@TyHolc TyHolc deleted the no_xdk_build branch October 26, 2023 22:23
@TyHolc TyHolc added the cp-24.lts.1+ Cherry Pick to the 24.lts.1+ branch label Oct 26, 2023
cobalt-github-releaser-bot pushed a commit that referenced this pull request Oct 26, 2023
Make references to the XDK internal-only to allow the external version
to build properly.

b/296290769

Change-Id: I3bf4c59de1fd3163028d713586294a2a93f403be
(cherry picked from commit e7bcf65)
TyHolc added a commit that referenced this pull request Oct 30, 2023
…1856)

Refer to the original PR: #1342

Make references to the XDK internal-only to allow the external version
to build properly.

b/296290769

Change-Id: I3bf4c59de1fd3163028d713586294a2a93f403be

Co-authored-by: Tyler Holcombe <[email protected]>
Rongo-JL pushed a commit to Rongo-JL/cobalt that referenced this pull request Dec 19, 2023
Make references to the XDK internal-only to allow the external version
to build properly.

b/296290769

Change-Id: I3bf4c59de1fd3163028d713586294a2a93f403be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cp-24.lts.1+ Cherry Pick to the 24.lts.1+ branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants