-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Local dev props #48739
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
base: main
Are you sure you want to change the base?
Local dev props #48739
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds documentation to simplify the process of consuming locally built Roslyn packages.
- Provides step-by-step instructions for checking out the Roslyn repository, copying necessary property files, building Roslyn, and restoring packages.
- Introduces a tip on speeding up package restore by clearing the cache and selectively applying local development settings.
Files not reviewed (4)
- Directory.Build.props: Language not supported
- Directory.Packages.props: Language not supported
- documentation/local-dev/roslyn/LocalDev.Build.props: Language not supported
- documentation/local-dev/roslyn/LocalDev.Packages.props: Language not supported
<!-- If \LocalDev.Packages.props exists, it will be loaded and merged with Directory.Packages.props, allowing for local adhoc build customizations. --> | ||
<PropertyGroup> | ||
<!-- Use the local version of Roslyn --> | ||
<RoslynVersion>4.14.0-dev</RoslynVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to update these versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How? These are locally built versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I was thinking that if it's built locally, we could perhaps overwrite something in this file as part of the build process? That only really works if it's in a fixed location and we just conditionally import it rather than having it normally live in documentation as here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could potentially fish out the version from the other repo, using RoslynRoot
set in LocalDev.Build.props. Setting RoslynVersion in LocalDev.Packages.props is easy enough, one time step though.
|
||
<!-- Import any local dev packages if they exist. --> | ||
<PropertyGroup> | ||
<LocalDevPackagesPath>$(MSBuildThisFileDirectory)LocalDev.Packages.props</LocalDevPackagesPath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems weird to me to have LocalDev.Packages.props and LocalDev.Build.props start in 'documentation' (which shouldn't have code, in my opinion) and need someone to copy them to the root. Perhaps we could have both always in the root (or, better yet, root/LocalDevTools or just root/eng) and import them conditionally if -p:SomeProperty=true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They need to be available when building from VS.
Also, these are just templates. They need to be updated to point to the local Roslyn repo and set the local version. That information is specific to the dev machine.
|
||
4. Build Roslyn with: | ||
|
||
`..\roslyn\Build.cmd -restore -pack` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a simpler approach (which is what I do currently and it works great):
- Build Roslyn:
.\build.cmd -pack /p:VersionSuffix=$(git rev-parse --short HEAD)
- In SDK's Versions.props: find+replace (simple VSCode action) current roslyn version with the built version
- Restore/build SDK.
That's it. No need to clean .packages
. No need to keep the list of Version properties in LocalDev.Packages.props in sync with Versions.props (if we add more roslyn dependencies over time). Works in other repos too (like runtime or razor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not simpler. I don't need to build and pack the entire Roslyn from command line, which takes a long time. I can pack just one project from VS. I don't need to update version every time I make a change. I only need to delete Roslyn packages from .packages dir and restore. Ideally, I would not want to do that either. I wonder if we can set the VersionSuffix automatically to what you do when building locally. Then in LocalDev.Packages.props we would use x.y.z-dev-*
pattern.
No need to keep the list of Version properties in LocalDev.Packages.props
If you update the version in Versions.props you need to revert that change before committing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't need to build and pack the entire Roslyn from command line, which takes a long time. I can pack just one project from VS.
I see, the doc says build.cmd -pack
so I assumed that's what you are doing.
I don't need to update version every time I make a change. I only need to delete Roslyn packages from .packages dir and restore.
Both are a single step, I don't think one is more difficult than the other.
But it's true that re-building just a single package with my approach isn't feasible. Your approach looks better in this scenario. Thanks for explaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another issue with using $(git rev-parse --short HEAD)
is that you need to remember to commit each time you want to switch to the other repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setting VersionSuffix to current time-based value would make it simpler still. Might explore that later on.
|
||
5. Clear the customized packages cache in `.packages` | ||
|
||
`git clean -dxf .packages/` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using git clean
and not simply rm -r
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way works. I just delete the directory from file explorer. @sharwell authored the original doc.
SDK Triage: This doesn't impact us and we can't think of a good reason not to have it. I'm not sure how many folks outside of tmat will ultimately use this but there's limited harm in having it in. We discussed if the files should be in the doc folder or not but it's better to be it next to the readme than elsewhere so this is fine. So once y 8000 ou have signoff, you're clear to merge from our team. |
Adds infrastructure that makes it easier to consume locally built Roslyn packages.
Adding
LocalDev.Build.props
andLocalDev.Packages.props
to the repo root redirects package restore to.packages
dir and sets version of Roslyn packages (or any other package) to locally built one.Based on @sharwell's work in another repository.