Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Update issue guide #8728

Merged
merged 2 commits into from
Dec 28, 2016
Merged

Update issue guide #8728

merged 2 commits into from
Dec 28, 2016

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Dec 27, 2016

Revert recent edit that recommended to track all issues in managed code in CoreFX repo

@jkotas
Copy link
Member Author

jkotas commented Dec 27, 2016

@karelz @danmosemsft PTLA

@danmoseley
Copy link
Member

/cc @stephentoub

[My bad for making the original edit (?) I thought it was based on consensus reached in some email which I can't find now]

If I understand right your motivation for corelib issues in CoreCLR is

  1. the issue may not be relevant to CoreRT
  2. it can be convenient to keep issues close to the code (although this one is fuzzy to me as few people only concern themselves with with bugs in one or the other)

My concerns as you know are --

  1. it is not always clear to folks especially community folks where the issue lies, so it will often go in the "wrong" repo. Primarily in CoreFX where center of mass is.
  2. folks only look at the CoreFX repo whether triaging or looking for relevant issues. it is cumbersome to have to search in both repos, and monitor both repoes. CoreCLR issues will always get less attention: issues I moved to CoreFX recently quickly got attention but had been neglected for months before.
  3. the connection between fix and repo is often not close. Corelib bug fixes usually need tests in CoreFX. Corelib API additions need exposing in CoreFX.
  4. what to do when issues go in the wrong repo? leave them (so both repos must always be examined and situation is inconsistent) or move them (which is one click -- but busywork -- and they may need moving back after investigation).
  5. managed=CoreFX is a simpler story and to date we see few corelib issues got opened in CoreCLR so it is practical to move them to CoreFX if they get opened there

I don't feel super strongly though so I defer to the majority! One option - leave as is (this edit is fine) and revisit in 2 or 3 months.

@jkotas
Copy link
Member Author

jkotas commented Dec 27, 2016

it is not always clear to folks especially community folks where the issue lies

I agree that some issues are hard, take time to trace down to the root cause, bouncing between teams and repos along the way. A lot of issues are simple and having the issue in the repo where the relevant code lives is the most straightforward rule.

CoreCLR issues will always get less attention

We have started actively managing issues in corefx just recently. The same needs to happen in coreclr too. I do not see why coreclr issues should get less attention.

what to do when issues go in the wrong repo?

Move them to the right repo. It is not unusual to see ASP.NET or .NET Core tooling issues opened in corefx, and vice versa. They get moved to the right repo too.

managed=CoreFX is a simpler story

Does managed means implementation language? A good chunk of core runtime parts are implemented in managed code in CoreCLR. CoreRT pushes it even further - everything except JIT and GC is implemeted in managed code. For example, dotnet/corert#2359 is likely bug in managed code (https://github.com/dotnet/corert/blob/master/src/Runtime.Base/src/System/Runtime/ExceptionHandling.cs). It would not make sense for it to live in corefx. I do not think sorting issues based on implementation language makes sense.

I think that the problem is that we do have some coupling between the repos structure and the organizations structure, but the mapping is not 1:1. karelz and danmosemsft teams tend to work in corefx, but not exclusively. gkhanna team tends to work in coreclr, but not exclusively. We are kind of wishing to have logical uber bug database for everything in .NET Core to get the different filters and global view that we are used to from TFS. We do not have it and trying to creatively workaround it.

@danmoseley
Copy link
Member

Fair enough. I do think we will be moving issues from CoreFX quite often but perhaps we don't need to be super strict/responsive about that.

I am not aware of a tool that allows you to see the familar Github list of issues aggregated from multiple repoes. It would be great to discover one. If you have Zenhub installed you can see them in board form eg this shows all System.Runtime issues from CoreCLR and CoreFX.

If we have such a tool, we would likely need consistent labels. Presumably most issues in corelib would map to System.Runtime and maybe that's okay.

@danmoseley
Copy link
Member

Github itself does allow searching issues across more than one repo, but it is not in the nice list format either.

Example System.Runtime issues in corefx or coreclr

@karelz
Copy link
Member

karelz commented Dec 27, 2016

FYI:
I don't have strong opinion either way. I think both systems are fine.
I don't like shipping org structures, however I want to make it easy for area experts to watch over their issues, so that we do not ignore issues for months/years. That is currently not well streamlined in coreclr repo - something I expect to follow up in Jan/Feb (finishing triage, set up alerts, rethink coreclr area labels, etc.).

@jkotas jkotas merged commit 0c7347a into dotnet:master Dec 28, 2016
@jkotas jkotas deleted the issue-guide branch December 28, 2016 06:45
@danmoseley
Copy link
Member

@jkotas I have a related question. Are there any, or many, types in corelib whose implementations could reasonably be moved up into corefx? That would reduce the amount of separated code, and help CoreRT also.

What determines whether something must be in corelib itself -- obviously where something is partially implemented in the runtime like String and Array, what else?

/cc @weshaggard

@jkotas
Copy link
Member Author

jkotas commented Dec 29, 2016

What determines whether something must be in corelib itself

Transitive closure of dependencies. For example, other things in runtime and corelib need file I/O -> file I/O should be in corelib too.

Pretty much everything that can be reasonably moved from corelib to corefx has been moved already. The few remaining large chunks that I think may be good candidates for move are:

  • reflection.emit: Requires non-trivial work (https://github.com/dotnet/corefx/issues/4491)
  • eventsource: The implementation for ProjectN/CoreRT is in corefx. Borderline case. It is unclear to me whether it is a good idea to move to corefx.

Otherwise, there may be a few types that we may be able to move, but nothing substantial.

In the past, we have done unnatural acts to move things from corelib to corefx (e.g. for file I/O), but I do not think they were improvement. They required duplicating the implementation, or setting up complex callbacks. Number of them fired back as part of NS 2.0 work and had to be undone. No point in going there again.

I agree we need a more effective scheme for sharing code between coreclr and corert, I believe that this scheme needs to be based on source code sharing e.g. via git submodules, not binary sharing. If we go with git submodules, corefx repo is not a good vehicle for it because of it is big and getting even bigger.

@jkotas
Copy link
Member Author

jkotas commented Dec 29, 2016

I agree we need a more effective scheme for sharing code between coreclr and corert

Note that this does block the refactoring to make coreclr and corert implementations same or similar. #8507 or #8720 (comment) are good recent examples.

@karelz karelz modified the milestones: 2.1.0, 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Revert "Update IssuesFeedbackEngagement.md"

This reverts commit dotnet/coreclr@124754f.

* Update wording


Commit migrated from dotnet/coreclr@0c7347a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants