-
Notifications
You must be signed in to change notification settings - Fork 334
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
DynamicDependency - Skeletal outline #158
Conversation
9a10b18
to
b9dcbae
Compare
Can we pull in detours as a submodule instead? What were the changes made to support Reunion in Detours? |
I'd like to avoid submodules, they're kind of a pain. I would have suggested nupkg of source or vcpkg -- looks like @DrusTheAxe had a conversation with detours folks about this (microsoft/Detours#124). Sounds like someone made a vcpkg of detours but it requires that you have vcpkg available. A copy of the source until vcpkg is integrated into the VS install seems ok. |
I have a comment to make on the source code organization discipline of this project. Having one vcxitems file in a subfolder of the dev directory per feature does not scale IMHO, and as #157 shows, this repo is going to get a whole lot bigger given enough time. While I understand the benefits of this approach (fewer files for apps to have to deploy), I would recommend that we use static libraries and the |
A few approaches were considered but this seemed a good start. We're still learning which choices are effective and which seemed like a good idea at the time but... One of the tenets of the Project Reunion effort is to be more flexible than traditional development of Windows, which sometimes means rewriting things (like, say, project structure...). But we need to get that first hand experience to evaluate what's not working and pick a better path. I share your reservations with the current structure (and I've got the bloody wounds to prove it...). I don't see some of the current lasting long term. I've already made some changes and I expect more to come (by myself and others), but...you gotta start somewhere. In the crawl/walk/run lifecycle we're still in the delivery room. Your static lib suggestion is interesting. I can see a few other options, though the tradeoffs are a bit fuzzy yet. Maybe I need more bloodletting (or maybe I'm woozy from current blood loss ). I'm aiming to get DynamicDependencies stood up and in master this month. After that, reviewing pain points and proposing changes, including project structure and other technical details, as well as non-technical process issues. I've made a note to include your comment in that list of things to re-evaluate. The journey of a thousand miles starts with a single step. We're far closer to that 1st step than the 1,000th but we're working on it. Please keep the feedback coming, but also be patient as we work to build a better world. R(ome)union won't be built in a day :-) -Howard P.S. Release configs should already have good optimizations enabled (I thought /OPT:ICF was already enabled, for one). That should be true regardless of the source structure how we combine the bits into end files. If you find oversights or inconsistencies please speak up. I'm hoping we get to a more automated structure but short term there's a lot of manual effort as we bootstrap the system. I'm playing whack-a-mole as I spot things but suggestions are (always) welcome |
How much of this is real product work vs. ES work? I'm leery of merging something that looks done (your product work) but isn't. Could some of the ES work be moved out to another branch temporarily while that finishes up? |
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 a "user's manual" for how another app would be able to generate their own lifetime manager etc.?
Feels like there's some common code we could do here, like "put this markup in your sxs application manifest and we'll find it" ... could even use a form of <dependency><packageDependency><package familyName="...."/>
then the DLL that they carry with them (the mdd seperate DLL) knows how to find and process this node to do all the dependency stuff. That could be optional, of course, so the app just calls MddAddDefaultDependencies()
if they've done the configuration.
All of my work is real product work, as in product code, or test code to verify product code. That happens to include some ES-y things e.g. make new projects for my tests, with the right compiler settings etc. But those are means to my ends, only done out of necessity to finish my work. Anything that smacks of straight ES I've largely been looking to see the fruits of @EHO-makai 's work :-) We'll eventually have some things we can refine, polish or simplify, and we are cross pollinating as we find problems the other's found solutions for. But so far we're pretty parallel/independent and focused on our lanes. |
PR is complete for review. Several places are marked with //TODO; will be addressed in upcoming PR. Would like to get signoff and complete this PR in the 'commit early and commit often' approach. Future PRs should be more bite-size given this foundation (easier to review, easier to change, faster code velocity, yadda yadda) |
} | ||
|
||
//TODO: Implement MddAddPackageDependency | ||
RETURN_HR(E_NOTIMPL); |
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.
You stated that this PR is "ready for review", but if so, why isn’t this implemented? Inquiring minds want to know. 😄
STDAPI_(void) MddDeletePackageDependency( | ||
_In_ PCWSTR /*packageDependencyId*/) | ||
{ | ||
//TODO: Implement MddDeletePackageDependency |
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.
Ditto.
*packageDependencyId = nullptr; | ||
|
||
//TODO: Implement MddTryCreatePackageDependency | ||
RETURN_HR(E_NOTIMPL); |
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.
Ditto.
96e4c8c
to
073912a
Compare
…s in some flavors; working on the rest
… #if defined(_ARM_) but VC++ defines _M_ARM_; should we just /D_ARM_ for ARM builds? ARM64=Fail because of some problem in dlldata.c. x86=Fail because 'LNK2001unresolved external symbol _ObjectStublessClient3' in the DynamicDependency.DataStore.ProxyStub project; need to #define something (_MERGE_PROXYSTUB)? Discussing with folks who know more
…blem; defaults to NT60 which 'doesn't need to link with ole32 for ObjectStublessClient'. How cryptic
…ng vs error (outofmemory, etc)
…etected; x64 and compiler didn't
…n/architecture (not checked in an shared across)
…s to play nice (gah!). Now to update the tests to match
…ager <sheepish grin>. Twewaking tests to play nice with DDLM
…and generally useful for debugging, troubleshooting and testability. Implemented ScopeIsSystem. Cleanup up some leftover cruft
Branch is wonky given the shift to main. I'm going to abandon this PR and make a new one rebased against main |
This is not complete yet but it builds cleanly and has the right structure. Sharing as other devs working on similar things coming up right behind me are stumbling over some of the same pains (or don't realize yet how ProjectReunion.sln needs to be refined)
What's new:
\dev
\test
Plus some other random things (fixed up ARM[64] project snippets etc)
What's not present:
\dev
\test
These will be addressed in upcoming PRs
Observations:
<Target>
snippet. Defined generically via macros so simple copy/paste job