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

Duplicate Items are logged when KeepDuplicates="false" #9585

Open
KirillOsenkov opened this issue Jan 3, 2024 · 4 comments · May be fixed by #10820
Open

Duplicate Items are logged when KeepDuplicates="false" #9585

KirillOsenkov opened this issue Jan 3, 2024 · 4 comments · May be fixed by #10820

Comments

@KirillOsenkov
Copy link
Member

<Project>

  <ItemGroup>
    <Item Include="A;B;B" KeepDuplicates="false" />
  </ItemGroup>

  <Target Name="Message">
    <ItemGroup>
      <File Include="@(Item)" KeepDuplicates="false" />
    </ItemGroup>
    <Message Text="@(File->'%(Identity)', ' ')" />
    <Message Text="@(Item->'%(Identity)', ' ')" />
  </Target>

</Project>

We log the File item as A B B however only A and B are actually added. I think we must be logging before the duplicates are removed?

image

@KirillOsenkov
Copy link
Member Author

Also, on line 4, should KeepDuplicates="false" deduplicate A;B;B to just A;B?

@KirillOsenkov
Copy link
Member Author

If it all worked properly, this would be a nifty replacement to the RemoveDuplicates task.

@SimaTian
Copy link
Contributor

SimaTian commented Oct 14, 2024

@KirillOsenkov ad

Also, on line 4, should KeepDuplicates="false" deduplicate A;B;B to just A;B?

according to this, no:

If an item is generated within a target, the item element can contain the KeepDuplicates attribute.

The Item you reference is not within a target. I just got burned by this exact issue while experimenting with the flag. I'm not sure why this distinction exists however removing it could be a breaking change.
Do we want to do something about this or is this something that needs to stay please? (I'm not aware enough of the context from which this distinction appeared to make a judgement)

@SimaTian
Copy link
Contributor

Already tracked under this which is a larger refactoring effort.

I've got the logging fix ready and I will be submitting a PR shortly. However that is only a half fix. (Fixing small cosmetic issue within the scope of this ticket, leaving rest unattended)

Possible follow up:

  • creating a built time opt in check (to not emit additional warning and still have a better visibility) to point out that KeepDuplicates within an Item outside a target is treated as a normal metadatum and doesn't do anything about duplicates. (Too late for making it reserved keyword since that could break people due to an error/warning emission)
  • going through with the unification of the behavior.
    • create a common point for remove duplicates functionality
    • retarget the inside of the target evaluation to it
    • figure out a proper place somewhere between ProjectParser.cs and Evaluator.cs. Probably around here, though it will need support in the parser as well.
  • making the documentation more explicit about this behavior. Currently it is "clear" english wise, but kind of fuzzy as in "not screaming large enough about a possible caveat"

@SimaTian SimaTian linked a pull request Oct 15, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants