-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Devise better way to build and publish samples from service directories #21204
Comments
I do think we can probably create a samples props/targets file that we can use for samples but we want them to mostly act like standard project defaults and not be treated like IsClientLibrary because that ends up being too coupled to our engineering system. |
But I think being coupled into our engineering system when built within our repo is exactly what we want. We want all the same analyzers, build, and possibly even test processes to run (I do have tests for one of my samples), but hide all that when built isolated from our repo, like when downloaded from Samples Browser. With Directory.Build.{props, targets} in sdk/keyvault/samples I've tried to do that as much as possible, but there are still some artifacts. There are also some interesting cases like sdk/keyvault/samples/sharelink where I have to copy linked friles to a directory or include them for the sample. Since @pakrym moved almost all autorest and core-shared code to a nupkg that list because SO much smaller, at least. What I devised in that case I think is worth generalizing as well. |
From my prospective the samples should mostly be built like they are outside of our engineering system, as if they were pulled out of our repo and built independently. |
There's no reason they can't "feel" standalone and still be built within our engineering system. That's why we made our samples not only compile but run as part of our nightlies. #7189 was seemingly the start of that process of making sure samples actually worked, and eventually we extracted from those into READMEs and the like to make sure whatever we showed customers as samples, best practices, etc., actually did what they were supposed to. Why would these samples be any different? What I did with sdk/keyvault/samples was, as much as possible given what we have now, make them work with our engineering system but also build externally. I tested all of them that they do work when isolated. But building them in our CIs, etc., is precisely the point. Even running our Azure analyzers makes sense, IMO. We write those to help guide customers into best practices using our code. I'm sure we don't all have all those best practices memorized, so running them during builds is also beneficial. |
@heaths do you still value in exploring this? What would be the benefit of doing this? Any particular use case you have in mind here? |
Pretty much the same resolution I gave for closing #24070: Key Vault established a pattern others have copied and can continue to copy, and given other priorities, this probably doesn't rise much beyond ground floor. |
/cc @jsquire |
The original work that went into sdk/keyvault/samples and the effort required to resolve #21170 required a lot of understanding and discovery of all our MSBuild properties and targets as well as MSBuild projects in general. This is not ideal for onboarding new teams who want to publish non-package-specific samples or, as in the case of these Key Vault samples, some which use multiple Key Vault packages and don't really fit in a single one.
Ideally, I think we could probably change a few of the item groups in eng/services to also treat
sdk/$(ServiceDirectoy)/samples
as samples and subject them to all the same rules as normalIsClientLibrary=true
packages, but treat them as non-shipping automatically.General props and targets also need to consider that samples may be
OutputType=Exe
and cannot use "netstandard2.0", which some targets seem to assume and required significant workarounds, understanding howTargetFramework
andTargetFrameworks
relate to each other./cc @pakrym @chidozieononiwu @weshaggard
The text was updated successfully, but these errors were encountered: