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

Deduplicate project files with the same name in different folders #64841

Merged
merged 2 commits into from
Mar 25, 2022

Conversation

trylek
Copy link
Member

@trylek trylek commented Feb 5, 2022

This is a somewhat annoying aspect hitting less than 10% of the
JIT/Methodical subtree: as the new merged wrapper logic is based
on simple assembly names, we cannot merge multiple test projects
that produce a test assembly with the same name otherwise such
assemblies stomp over each other when getting copied to the merged
wrapper folder. I have added a new option to ILTransform to include
the directory name in the project names in these cases. We can
remove some of this in the future when selectively merging actual
test source code (compiling multiple tests into a single asssembly).

Thanks

Tomas

/cc @dotnet/jit-contrib

@trylek trylek added this to the 7.0.0 milestone Feb 5, 2022
@ghost ghost assigned trylek Feb 5, 2022
@ghost
Copy link

ghost commented Feb 5, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

This is a somewhat annoying aspect hitting less than 10% of the
JIT/Methodical subtree: as the new merged wrapper logic is based
on simple assembly names, we cannot merge multiple test projects
that produce a test assembly with the same name otherwise such
assemblies stomp over each other when getting copied to the merged
wrapper folder. I have added a new option to ILTransform to include
the directory name in the project names in these cases. We can
remove some of this in the future when selectively merging actual
test source code (compiling multiple tests into a single asssembly).

Thanks

Tomas

/cc @dotnet/jit-contrib

Author: trylek
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: 7.0.0

@trylek
Copy link
Member Author

trylek commented Feb 5, 2022

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@trylek trylek closed this Feb 7, 2022
@trylek trylek reopened this Feb 7, 2022
@trylek
Copy link
Member Author

trylek commented Feb 7, 2022

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

This seems kind of annoying, though I suppose minor in the scheme of things.

Once again, I worry about when we go to create a new test later, we can't just create a test "context free" -- it has to obey a set of rules about naming that are global to the repo, which is unfortunate, but hopefully very easy to check. E.g., we normally create a new test by copy/paste/edit from an existing test. Now, we won't know if that name is ok until outerloop tests are run (presumably just building/running Pri-0 won't be sufficient to ensure naming rules are followed, if a new Pri-0 conflicts with a Pri-1 test, for example).

Also:

we cannot merge multiple test projects
that produce a test assembly with the same name otherwise such
assemblies stomp over each other when getting copied to the merged
wrapper folder

Hopefully the copy logic will check that there is no stomping, so there will be a noisy failure, not just a silent stomping?

@trylek
Copy link
Member Author

trylek commented Feb 9, 2022

@jkoritzinsky - regarding Bruce's (reasonable) concerns, would it be possible to somehow detect the silent overlap of two test reference assemblies with the same simple name in the merged wrapper generator and produce and actionable error message? This PR demonstrates a plethora of cases where this is happening but we have no safeguards today, the tests in question just silently disappear and I agree with Bruce that is wrong.

@jkoritzinsky
Copy link
Member

I'm not sure how much control we have over that, as the name clashes are silently handled by the ResolveAssemblyReferences task. We could add in our own target that runs after RAR that validates that each ProjectReference resulted in an assembly reference and emit an error in case of a missing one due to RAR resolving a name conflict.

@trylek
Copy link
Member Author

trylek commented Feb 9, 2022

Thanks Jeremy for your feedback; I would however expect that these should boil down to differing project references as the project paths are different, it's just that the output assemblies happen to have the same filename and so they cause an overlap when copying them to the merged wrapper output folder. I'll take a more detailed look at the binlog tomorrow to see what exactly happens in the scripts and how to implement the safeguards I proposed.

@trylek trylek closed this Mar 19, 2022
@trylek trylek reopened this Mar 19, 2022
@trylek
Copy link
Member Author

trylek commented Mar 19, 2022

/azp run runtime-coreclr outerloop

@trylek
Copy link
Member Author

trylek commented Mar 19, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@trylek
Copy link
Member Author

trylek commented Mar 19, 2022

To cleanly resolve this issue and address Bruce's concerns, Jeremy is working on adjusting build of IL projects by replacing hardcoded .assembly directives in the IL source code with generated ones. I believe this to be a reasonable long-term solution to the problem and I plan to merge this in after retesting to finally open the door to merging in the switchover of JIT/Methodical tests to use merged wrappers.

@jkoritzinsky
Copy link
Member

In addition to generating the .assembly directives, I’m trying to implement a check in the build system to help with validation in case of duplicate names. I’ll hopefully be able to put both ideas out for review next week.

@trylek trylek force-pushed the JIT-Methodical-auto-project-dedup-2 branch from aef62a1 to d5788b5 Compare March 20, 2022 18:58
@trylek
Copy link
Member Author

trylek commented Mar 20, 2022

/azp run runtime-extra-platforms

@trylek
Copy link
Member Author

trylek commented Mar 20, 2022

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member

I've opened a PR with the validation target at #66951. That PR should catch any cases of missed coverage.

I don't think auto-generating the .assembly directives will help stop clashes as the auto-generated ones would probably be very similar to the ones in this PR.

This is a somewhat annoying aspect hitting less than 10% of the
JIT/Methodical subtree: as the new merged wrapper logic is based
on simple assembly names, we cannot merge multiple test projects
that produce a test assembly with the same name otherwise such
assemblies stomp over each other when getting copied to the merged
wrapper folder. I have added a new option to ILTransform to include
the directory name in the project names in these cases. We can
remove some of this in the future when selectively merging actual
test source code (compiling multiple tests into a single asssembly).

Thanks

Tomas
@trylek trylek force-pushed the JIT-Methodical-auto-project-dedup-2 branch from d5788b5 to 3e644e1 Compare March 25, 2022 09:10
@trylek
Copy link
Member Author

trylek commented Mar 25, 2022

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@trylek trylek merged commit 93957c6 into dotnet:main Mar 25, 2022
@trylek trylek deleted the JIT-Methodical-auto-project-dedup-2 branch March 25, 2022 19:16
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
…tnet#64841)

This is a somewhat annoying aspect hitting less than 10% of the
JIT/Methodical subtree: as the new merged wrapper logic is based
on simple assembly names, we cannot merge multiple test projects
that produce a test assembly with the same name otherwise such
assemblies stomp over each other when getting copied to the merged
wrapper folder. I have added a new option to ILTransform to include
the directory name in the project names in these cases. We can
remove some of this in the future when selectively merging actual
test source code (compiling multiple tests into a single asssembly).

Thanks

Tomas
@ghost ghost locked as resolved and limited conversation to collaborators Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants