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

Multi TFMs build #14738

Closed
wants to merge 18 commits into from
Closed

Multi TFMs build #14738

wants to merge 18 commits into from

Conversation

jtkech
Copy link
Member

@jtkech jtkech commented Nov 22, 2023

@sebastienros

As commented in #14732, if we apply #14732 and if we support multiple TFMs, when building by specifying a TFM it only copy the translation files if the specified TFM is the last one.

So here I tried the following changes, maybe worth to try them on your machine

  • Here we don't set our IsCrossTargetedBuild in a props file but through a new Target.

    This new Target is not called when the Tests project is built, so our custom property is not set in this context. In this context the translation files are copied per TFM but it was the case before.

  • Then I simplified the condition of the Target copying the translations files.

If it doesn't fix the issue, I suggest to just copy the files per TFM by removing the condition, I don't think that it really matters as we skip unchanged files.

Note: In this PR I re-added net7.0 so that it is easier to test, will need to be removed if we apply it.

@@ -9,7 +9,7 @@

<!-- TFMs used to build the abstractions and modules, by convention the default TFM is at the first position -->
<!-- In a cross-targeting build, some assets are only copied on the first TFM, by convention the default TFM -->
<CommonTargetFrameworks Condition="'$(CommonTargetFrameworks)' == ''">net8.0</CommonTargetFrameworks>
<CommonTargetFrameworks Condition="'$(CommonTargetFrameworks)' == ''">net8.0;net7.0;net6.0</CommonTargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Did we bring the previous versions back?

Copy link
Member Author

Choose a reason for hiding this comment

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

No ;) This for testing race conditions

Copy link
Member

Choose a reason for hiding this comment

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

I see, but why the previous TFM are there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because race conditions happen when building multiple TFMs at the same time

@jtkech
Copy link
Member Author

jtkech commented Nov 26, 2023

@sebastienros just for info

I know that you are investigating on this issue and possible other ones but I did some tests in this PR.

I had different issues, one of them when generating xml files on GenerateDepsFile, as I remember more often under Windows, but only when mutating Properties of inner projects even if done in a Target.

So for now I think we better have to always copy translation files without any condition knowing that we skip unchanged files.

Note: Would be good that they provide a property accessible by inner projects, to know that we are building an inner project.

@sebastienros
Copy link
Member

@jtkech I updated my PR, then looked at this one, and I see you are arriving to the same conclusion as me. So many hours just for that ;)

@jtkech
Copy link
Member Author

jtkech commented Nov 28, 2023

No problem, it was interesting, yes they should provide a property accessible by inner projects, to know that we are building an inner project, but that's fine to "always" copy translation files.

@jtkech
Copy link
Member Author

jtkech commented Nov 30, 2023

Close, see #14732

@jtkech jtkech closed this Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants