search

BHoM / admin

Repository for raising central issues and questions; Regarding governance, process and multi-repo and framework compliance
GNU Lesser General Public License v3.0
0 stars 0 forks source link

Update of toolkit postbuild and project references strategy #5

Closed al-fisher closed 4 years ago

al-fisher commented 4 years ago

As discussed benefits now to revisiting our approach to references to BHoM .dlls and management of toolkit responsibility for copying files to central user/program data location.

A few related steps to get agreement on, before potentially making changes across all Toolkits. At point of making changes we should also add new required compliance check in CI for all toolkits (all that are not pre-alpha and are in installers at least)

  1. Suggesting change location for installer and loading BHoM dlls to C:\ProgramData\BHoM\Assemblies This is naturally not unique to users, thus simpler to hardcode references to. Big advantage. One thing to note. You do not need admin rights to write to ProgramData. Although other users will potentially need admin rights if using same machine to modify or uninstall BHoM once installed. Think this is fair - with a benefit to making BHoM version not user specific

  2. Switch to referencing BHoM .dlls in Toolkits directly from the centralised ProgramData location (rather than relative Build folders as we are doing now) Benefit here is huge as discussed - as user/developer can keep code references up to date via installers. Do not need to clone all dependencies and build just to work on derivative toolkit.

  3. Devolve responsibility for copying files to ProgramData back to individual toolkits and repos. i.e. BHoM_UI will no longer have a PostBuild.exe, blanket mining all conforming folders. This is now possible to maintain easily, by including clear required CI check in all PRs.

  4. One thing to consider carefully is the copying of Datasets across. These will logically sit in C:\ProgramData\BHoM\Datasets. We previously moved responsibility for this action to BHoM_UI (for good reason) Need to revisit. Is it still BHoM_UI, or likely return to the Core BHoM_Dataset being responsible.

  5. Similarly - worth taking this oppertunity to confirm best location for BHoMUpgraders. Opportunity to promote to clear top level folder, such that we have:

C:\ProgramData\BHoM\Assemblies
    \Assemblies
    \Datasets
    \Upgrades

@FraserGreenroyd @adecler @IsakNaslundBh @rwemay Some points still to discuss above there and get good agreement on approach. Thanks

rwemay commented 4 years ago

Looks very good to me. I built BHoM_UI yesterday for another PR/test, and this morning had another task to do with an unrelated bit of the code/tools that had broken - as is difficult to maintain all repo's sync'd. So two massive thumbs up from me!

al-fisher commented 4 years ago

Capturing issues to address as discussed with @IsakNaslundBh and others this morning.

A. Locking dlls will be more common & B. Strategy for current postbuild event mining upgraders in BHoM_UI

A. Locking dlls will be more common. By switching referencing all BHoM .dlls in Toolkits directly to the centralised ProgramData (i.e. building from same location as loading) overwriting .dlls already loaded in a UI will be prevented.

In practical terms this means Excel, Grasshopper etc. will need to be closed at the time of compiling any toolkit (not just at the time of compiling BHoM_UI as is currently the case)

Personally, and from discussions today, I think this is a fine compromise to facilitate the benefit of centralised .dlls. Building individual repos with say Excel open will be possible - just the post build event overwriting .dll will fail. So simply rebuilding to checking compilation errors etc. with UI open for single toolkits (a use case mentioned) is possible. Modifying chains of dependant toolkits (i.e. editing both BHoM_Engine and then GSA_Toolkit say) will not be possible. GSA_Toolkit will be pointing to locked .dll.

I think the loss of the fringe case is OK to facilitate the centralisation. But good to hear others thoughts as always?

Finally options to explore forced unloading of BHoM and .dlls from UIs. However challenges potentially with implementing consistently and so would not put this as critical for implementing this issues proposed changes.

B. _Versioning_Toolkit and current postbuild event mining upgraders in BHoMUI Currently there are a series of actions handled by the BHoM_UI UI_PostBuild.exe that collect and aggregate together all of the Versioning_vX.json files.

In the immediate term I think we have a couple of options which will simply require developers to do one of two things to maintain and ensure their versioning is up to date.

i. Continue to rebuild the BHoM_UI last - triggering the the remaining CopyUpgrades parts of the current UI_PostBuild.exe

or

ii. frequently use the latest alpha installer to get all the up to date .dlls, datasets and versioning files.

N.B. As discussed, this second is something we will be wanting to encourage more and more for developers anyway, so users and coders are using same end product.

A separate question is should we migrate responsibility for the current versioning post builds to the Versioning_Toolkit? So developers doing versioning will need to call Versioning_Toolkit instead?

In medium term it will be useful to explore if the actual post build tasks of converting raw .json and aggregating could be done at run time for the current live milestone. This would enable a similar fully decentralised approach to versioning - mush like Datasets where responsibility for moving to central location could be devolved to toolkits themselves as a very simple task. @adecler good to get your thoughts on this point - prob warrants a bigger discussion in detail.

At end of milestone we can then still "lockdown" the aggregated .json into binary - so no longer editable and start afresh.

If we think the above medium term plan is a good idea - I might suggest we just leave responsibility of versioning migration to BHoM_UI for these final sprints of 3.2 while we sort the above non-versioning related tasks. To look at wider plans for next improvements to versioning in 3.3

Thoughts @IsakNaslundBh @adecler all

(P.S. sorry for long post 📃 )

TL;DR

A. Don't worry too much about locking .dlls 😄
B. To version as a developer - keep up to date with alpha installers (and/or just call BHoM_UI as before)

pawelbaran commented 4 years ago

All sounds good 👍 One more potential issue just came to my head: it is a good practice to stick to a single BHoM build/version per project, for obvious reasons. This may become problematic once the assemblies will be shared by many users or admin rights will be required.

Possibly it could be worth considering to allow multiple BHoM versions per machine? Manifest files (.gha, .addin, etc.) could sit in AppData per user then, pointing to the user's preferred version in ProgramData.

First blocker I see for such approach is Dynamo, where all files are copied over to its dedicated folder. However, as far as I remember, there should be a way to hack the .dll paths - maybe @adecler could say more?

al-fisher commented 4 years ago

Thanks @pawelbaran - good point raised about having multiple parallel versions of BHoM installed simultaneously.

As discussed together - the use case of different versions of BHoM running simultaneously in different UI (say Excel vs GH vs Dynamo) - is actually prob one we want to explicitly avoid - to minimise unforeseen behaviour/user error.

With installers now relatively easy (and hopefully getting easier still!) to reinstall previous versions and upgrade/downgrade - suggesting we park parallel versions of BHoM for now.

A version of your proposed solution could be possible - but perhaps to be revisited if any future demand or user need is flagged. Hope makes sense?

adecler commented 4 years ago

Agreed with OP. This looks like a great simplification of the current solution so always up for that :smile:.

I would move versioning post build to the versioning toolkit. We're dealing with static json files here so build order is not an issue. There is just a new need to remember to build the versioning toolkit if the json files have been modified but I can live with that.

More and more I want to keep the location of the UI dlls a separate thing from the other dlls. Keeping UIs completely separate from each other is something that we want and even need to reduce issues of conflict and incorrectly exposing things from other UIs. This obviously doesn't mean that we have to copy all dlls to each UI folder like we do now. we could better leverage the PATH system variable for that.

al-fisher commented 4 years ago

Following this morning discussion - the following plan has been agreed for actioning this issue.

Pre-work

to be completed in advance of 13:00 UTC Wed 03 June

Update of individual UIs

Update of compliance checks to assist in upcoming PRs, as well as beyond

Workshop actions

to be completed from 08:30 UTC Thursday 03 June onwards

Post workshop and completion of this issue

New issues to be raised specifically for

al-fisher commented 4 years ago

Agreed branch name for this work is

BHoM_Admin-#5-Referencing

al-fisher commented 4 years ago

@adecler @FraserGreenroyd @IsakNaslundBh @awakeman @pawelbaran please see notes above to action plan as agreed this morning.

adecler commented 4 years ago
IsakNaslundBh commented 4 years ago
  • @IsakNaslundBh , I kept the target folder to Assemblies/bin to mirror what you did in the engine (ToNewVersion file) but it is actually very easy to use the BHoM/Upgrades folder instead as nothing changes in the code, only in the post build command. So happy to do the switch now.

Have updated https://github.com/BHoM/BHoM_Engine/pull/1823 to your suggestion here @adecler . Did change the folder to Upgrades without the bin.

al-fisher commented 4 years ago

On post-builds As agreed the responsibility for moving .dll is now that of the .csproj Specifically the project .dll itself $(TargetFileName)

But also any of its dependencies - NuGet acquired dlls or similar MUST also be copied over to C:\ProgramData\BHoM\Assemblies as an additional explicit line in the post build event.

standard default post-build command: xcopy "$(TargetDir)$(TargetFileName)" "C:\ProgramData\BHoM\Assemblies" /Y

Additional creation of system folders post-build events will be added to BHoM.csproj only

mkdir "C:\ProgramData\BHoM\Assemblies"
mkdir "C:\ProgramData\BHoM\Datasets"
mkdir "C:\ProgramData\BHoM\Upgrades"

to remind - this is because BHoM.dll is the primary dependency and thus is guaranteed to be built (or will come with installer). Thus no other toolkit should have to worry about creation of folders (only copying into them)

adecler commented 4 years ago

Have updated BHoM/BHoM_Engine#1823 to your suggestion here @adecler . Did change the folder to Upgrades without the bin.

Thanks @IsakNaslundBh . I have updated Prs on both BHoM_UI and Versioning_Toolkit to reflect that

IsakNaslundBh commented 4 years ago

standard default post-build command: copy "$(TargetDir)$(TargetFileName)" "C:\ProgramData\BHoM\Assemblies" /Y

@al-fisher this differs slightly from what I had put in place for the automated script:

xcopy "$(TargetPath)" "C:\ProgramData\BHoM\Assemblies" /C /Y

Remember having some issue with copy at one point, but cant remember exactly what it was and why. Trying to figure out the difference now, but should agree on something before actioning all the changes today!

@adecler @FraserGreenroyd

adecler commented 4 years ago

I used xcopy in Versioning toolkit because some of the options (like auto-creation of folders) make it easier and more concise.

al-fisher commented 4 years ago

As agreed - I have updated the standard default post-build command see above to: xcopy "$(TargetDir)$(TargetFileName)" "C:\ProgramData\BHoM\Assemblies" /C /Y

Agreement to standardise around xcopy to help with extending for exceptional cases ~and including /C switch also for robustness of process.~

Also agreed "$(TargetDir)$(TargetFileName)" separation of the two variables (rather than using the similar targetpath) is valuable for transparency.

al-fisher commented 4 years ago

Agreed removal of the /C switch to give louder error - rather than silent sharing violation.

Is updated above.

al-fisher commented 4 years ago

Pre-work and workshop actions as listed above in comment https://github.com/BHoM/admin/issues/5#issuecomment-637538563 have successfully been completed with all toolkits included in installers updated to reflect the required changes.

Post workshop actions still required naturally including further testing of alphas beyond that carried out today. Further issues to be raised for post-workshop actions.

Thanks @FraserGreenroyd @adecler @IsakNaslundBh @pawelbaran @awakeman @kThorsager @MajaLindroth @peterjamesnugent in particular for efforts today 😄 🎉