Skip to content
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

Change target framework and remove no longer required dependencies #81

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

IsakNaslundBh
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh commented Jun 27, 2024

Issues addressed by this PR

Closes #79

Changes the target framework of the debug and release build to simply be netstandard2.0 .

Whilst attempting to do this, realised that the reason this toolkit was still on 4.7.2 net framework was the VerifySecurityEvidenceForIsolatedStorage to handle saving of large files. From testing with file provided in BHoM/Excel_UI#331 this does not seem to be an issue any longer for netstandard2.0 , hence the method has been removed.

@pawelbaran would greatly appreciate if you are able to check that you are happy with this removal, given you raised this and fixed it in the first place.

Ofc general functionality of the toolkit should also be checked to ensure this does not have any other detrimental impacts on the toolkit.

Test files

Test file here

Changelog

Additional comments

@IsakNaslundBh IsakNaslundBh added the type:compliance Non-conforming to code guidelines label Jun 27, 2024
@IsakNaslundBh IsakNaslundBh self-assigned this Jun 27, 2024
@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance

Copy link

bhombot-ci bot commented Jun 27, 2024

@IsakNaslundBh to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check project-compliance

Copy link

bhombot-ci bot commented Jun 27, 2024

@IsakNaslundBh to confirm, the following actions are now queued:

  • check project-compliance

Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes make sense to me, but unfortunately Excel adapter does not work inside Revit on this PR, following exception is thrown:
image

Via trial and error I identified that it is DocumentFormat.OpenXml.Packaging.OpenXmlPackage type (a property of DocumentFormat.OpenXml.Packaging.SpreadsheetDocument) that cannot be loaded.

I was trying to track down the origins using Fusion Log, got this:
image

The above does not make much sense to me, why is DocumentFormat.OpenXml trying to load System.IO.Packaging 4.0.5, if it clearly depends on 6.0.0?
image

Meanwhile, I do not have any issues with loading types from System.IO.Packaging directly in Push method, e.g. adding System.IO.Packaging.Package p = null binds nicely to 6.0.0.

Going further, at the moment of calling Push, DocumentFormat.OpenXml 2.16 is already loaded into app domain from C:\ProgramData\BHoM\Assemblies\DocumentFormat.OpenXml.dll, which is alright.

So to summarise: everything seems to be perfectly fine, right assemblies being loaded and referenced, until DocumentFormat.OpenXml.Packaging.OpenXmlPackage is reflected, causing an attempt to load System.IO.Packaging 4.0.5 instead of 6.0.0. My gut feeling is that there is some voodoo happening due to the app domain being owned by Revit that sits on .NET Framework, but can't find out where & why. @adecler, any ideas what could be wrong here? Suggestions how to debug deeper?

@adecler
Copy link
Member

adecler commented Jul 2, 2024

@pawelbaran , my initial finding leads to SAP Toolkit being the potential problem as it still depends ClosedXML version 0.95:

image

Updating the version of ClosedXML in SAP Toolkit (PR here) seems to help, at least when looking at Fusion. It will have to be tested in Revit.

@pawelbaran
Copy link
Member

pawelbaran commented Jul 2, 2024

Thanks for help @adecler - unfortunately it does not fix Revit :< Just tried with BHoM/SAP_Toolkit#173, still getting the same exception

Copy link

bhombot-ci bot commented Nov 20, 2024

@IsakNaslundBh just to let you know, I have provided a check-installer result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @Tom-Kingstone on SAP_Toolkit

@pawelbaran pawelbaran force-pushed the Excel_Toolkit-#79-ChangeTargetFrameworkAndRemoveNoLongerRequiredDependencies branch from 52fd371 to dcbe810 Compare December 16, 2024 09:52
@pawelbaran
Copy link
Member

After the recent issue with System.IO.Packaging assembly raised to @michal-pekacki by the users, I had another think about fixing it - having succeeded with the initial testing, I rebased the branch and pushed the fix. Something to pick up early 8.1 I believe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:compliance Non-conforming to code guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ExcelAdapter always target netstandard2.0
3 participants