This repository was archived by the owner on Jan 23, 2023. It is now read-only.
This change make DacTableGen work for VS2017#17004
Merged
Merged
Conversation
There was a issue (see https://github.com/dotnet/coreclr/issues/11305) where a tool used in the CoreCLR build called DacTableGen requires a old version of the msdia120.dll. The symptom is that this tool would fail with a class not registered errror. This tool comes from a very old nuget package microsoft.dotnet.buildtools.coreclr, which we no longer can build easly. Our guidance now is to move tools that are only used in one repository (like this one) out of nuget packages and simply build them as part of the build. This change makes a step in that direction. The DacTableGen code actually was already in the CoreCLR repo, so this change 1. Fixes the source of DacTableGen so that tool no longer needs a com object to be registered (but it DOES need msdia140.dll to be on the path. This is true for VS2017. 2. Turns on the build of DacTableGen 3. Change the build use the built DacTableGen (unless running on VS2017, in that case we use the the version in the tool package. 4.) Remove the hack that warns people to register msdia120 (since you don't need to anymore) There is also an unrelated addition to the docs. This change should still work for VS2015 (because it falls back to the old DacTableGen in that case) Finally we should move to using the Linux method of creating the DAC, and so all these tools and their nuget package can be removed.
mikem8361
approved these changes
Mar 17, 2018
jkotas
reviewed
Mar 17, 2018
| These builds have a version number that follows the the versioning scheme described below (month number/day of month), | ||
| but they also will have a component that indicate which Git Branch the are working from | ||
|
|
||
| * preview1 - are daily builds from the 'release/*' branch where * is the next official version to be released |
Member
There was a problem hiding this comment.
* should be escaped so that it does not cause the rest to be italic
jkotas
reviewed
Mar 17, 2018
| but they also will have a component that indicate which Git Branch the are working from | ||
|
|
||
| * preview1 - are daily builds from the 'release/*' branch where * is the next official version to be released | ||
| * preview2 - are daily builds from the 'master' branch (where active work happens first (typically)) |
Member
There was a problem hiding this comment.
This is going to be true like for next <2 weeks, and then these suffixes are going to change again.
janvorli
reviewed
Mar 19, 2018
| clr_unknown_arch() | ||
| endif() | ||
|
|
||
| set(DACTABLEGEN_EXE ${CMAKE_CURRENT_BINARY_DIR}/../../../ToolBox/SOS/DacTableGen/dactablegen.exe) |
Member
There was a problem hiding this comment.
A nit - Instead of this deep relative path, the following should work: ${CMAKE_BINARY_DIR}/src/ToolBox/SOS/DacTableGen/dactablegen.exe
Author
There was a problem hiding this comment.
I did not like the ugly relative path, I will give that a try.
Author
|
cc: @jashook |
vancem
pushed a commit
to vancem/coreclr
that referenced
this pull request
Mar 19, 2018
vancem
pushed a commit
that referenced
this pull request
Mar 19, 2018
Use better path (fixes #17004)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a issue (see https://github.com/dotnet/coreclr/issues/11305) where a tool
used in the CoreCLR build called DacTableGen requires a old version of the
msdia120.dll. The symptom is that this tool would fail with a class not registered errror if
you build on a machine with only VS2017. This is blocking upgrading to VS2017.
This tool comes from a very old nuget package microsoft.dotnet.buildtools.coreclr, which
we no longer can build easly. Our guidance now is to move tools that are only used in
one repository (like this one) out of nuget packages and simply build them as part of the build.
This change makes a step in that direction. The DacTableGen code actually was already
in the CoreCLR repo, so this change
(but it DOES need msdia140.dll to be on the path. This is true for VS2017.
the version in the tool package.
4.) Remove the hack that warns people to register msdia120 (since you don't need to anymore)
There is also an unrelated addition to the docs.
This change should still work for VS2015 (because it falls back to the old DacTableGen in that case)
Finally we should move to using the Linux method of creating the DAC, and so all these tools and
their nuget package can be removed.
@mikem8361