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

Disable UpdateXlfOnBuild for VMR builds #17585

Merged
merged 7 commits into from
Aug 23, 2024
Merged

Disable UpdateXlfOnBuild for VMR builds #17585

merged 7 commits into from
Aug 23, 2024

Conversation

T-Gro
Copy link
Member

@T-Gro T-Gro commented Aug 22, 2024

This changes the condition set for UpdateXlfOnBuild.

Right now, net9 builds are occasionally failing with "The target "UpdateXlf" does not exist in the project".

However, updating localization files happens locally before contribution (see guidelines), it is not needed as a CI step.

This changes the condition set for UpdateXlfOnBuild.

Right now, net9 builds are occassionaly failing with "The target "UpdateXlf" does not exist in the project".

However, updating localization files happens locally before contribution (see guidelines), it is not needed as a CI step.
@T-Gro T-Gro requested a review from a team as a code owner August 22, 2024 08:49
@T-Gro T-Gro requested a review from akoeplinger August 22, 2024 08:49
@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Aug 22, 2024
Copy link
Contributor

github-actions bot commented Aug 22, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@vzarytovskii
Copy link
Member

I will be more interested in why are they failing.

@T-Gro
Copy link
Member Author

T-Gro commented Aug 22, 2024

I will be more interested in why are they failing.

The target is not correctly imported in case of net9, it is an issue within the UpdateXlf task.
Alex will follow on that.

image

@akoeplinger
Copy link
Member

Looks like the xlf files need to be updated now since CI is no longer inadvertently regenerating them.

You can also remove the SKIP_NETCURRENT_FSC_BUILD workaround from

On CI, OSX has problems with Xliff targets for net9, skipping via SKIP_NETCURRENT_FSC_BUILD until resolved ( The target "UpdateXlf" does not exist in the project.)
-->
<TargetFrameworks Condition=" '$(OfficialBuildId)' == '' AND '$(FSharpNetCoreProductDefaultTargetFramework)' != '' AND '$(Configuration)' != 'Proto' AND '$(SKIP_NETCURRENT_FSC_BUILD)' != 'true' ">$(FSharpNetCoreProductDefaultTargetFramework);$(TargetFrameworks)</TargetFrameworks>

@T-Gro
Copy link
Member Author

T-Gro commented Aug 22, 2024

Looks like the xlf files need to be updated now since CI is no longer inadvertently regenerating them.

You can also remove the SKIP_NETCURRENT_FSC_BUILD workaround from

On CI, OSX has problems with Xliff targets for net9, skipping via SKIP_NETCURRENT_FSC_BUILD until resolved ( The target "UpdateXlf" does not exist in the project.)
-->
<TargetFrameworks Condition=" '$(OfficialBuildId)' == '' AND '$(FSharpNetCoreProductDefaultTargetFramework)' != '' AND '$(Configuration)' != 'Proto' AND '$(SKIP_NETCURRENT_FSC_BUILD)' != 'true' ">$(FSharpNetCoreProductDefaultTargetFramework);$(TargetFrameworks)</TargetFrameworks>

I can remove the workaround for CI steps, I have to keep the option to disable it for local MacOs/Linux contributors.
@vzarytovskii experienced the error when building on MacOs locally as well IIRC.

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Hope it helps, thanks!

@akoeplinger
Copy link
Member

I can remove the workaround for CI steps, I have to keep the option to disable it for local MacOs/Linux contributors.
@vzarytovskii experienced the error when building on MacOs locally as well IIRC.

Thanks, @ViktorHofer was also able to reproduce the issue locally and is looking into it. It currently looks like it is somehow related to project.assets.json being used from the Proto Configuration and that one doesn't contain net9.0

@ViktorHofer
Copy link
Member

Yes. I would recommend to hold off on this change. I will look at it more tomorrow in #17588

@akoeplinger
Copy link
Member

This is a good change regardless since we shouldn't be setting UpdateXlfOnBuild=true in CI builds.

@ViktorHofer
Copy link
Member

Isn't the Arcade default sufficient? I don't recall other repos setting this.

@akoeplinger
Copy link
Member

Yes the default should work too.

@T-Gro
Copy link
Member Author

T-Gro commented Aug 23, 2024

Ok, will see if the default works as well here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants