OfficeDev / Open-Xml-PowerTools

MIT License
692 stars 26 forks source link

Add support for .Net Core #238

Closed PhenX closed 6 years ago

msftclas commented 6 years ago

CLA assistant check
All CLA requirements met.

tikal commented 6 years ago

@PhenX FYI, tests are failing

PhenX commented 6 years ago

I took your comments into account in the last commit, tell me if I forgot something :)

PhenX commented 6 years ago

Is it ok now ?

PhenX commented 6 years ago

Also, do you know when the Nuget package is able to be updated with these changes ?

vladimir-shmidt commented 6 years ago

Hi guys, any news about this? appveyor says that one test have been failed: [xUnit.net 00:00:04.23] OxPt.PbTests.PB006_VideoFormats [FAIL] Error Message: System.IO.IOException : Entries cannot be opened multiple times in Update mode. This error looks very the same as https://github.com/dotnet/corefx/issues/26952 and was fixed and merged in https://github.com/dotnet/corefx/pull/28478/commits/8797ecbffd8f050cb6cadff8f790fac6faf83bed

image have checked locally

possibly problem with appveyor. Maybe Runtime Core 2.0 installed but problem with failed tests resolved in Runtime Core 2.1

twsouthwick commented 6 years ago

@vladimir-shmidt Sorry for the delay - been swamped with stuff. Thanks for the fixes - I'll go through it again.

As far as the error, go ahead and change the tests to target .NET Core 2.1 and see if that fixes it. It's not ideal, as we should fix the tests to work correctly. I saw this often on the OpenXml SDK where streams were not being disposed of correctly. So, those are two options to fix it and I'll leave it up to you - I prefer fixing disposing if that's the issue, but if it's truly a .NET bug, then we won't worry about it.

twsouthwick commented 6 years ago

Ah - I misunderstood the issue you linked to. Looks like this is fixed for all versions, just in a later System.IO.Packaging. v2.9.0 of the SDK brings in the latest changes and should have it fixed. However, we've run into some issues putting it on the official nuget stream - it's only on the CI. You can update to v2.9.0 of the SDK and it'll be fixed, just need to add an entry in the NuGet.config for https://dotnet.myget.org/F/open-xml-sdk/api/v3/index.json

vladimir-shmidt commented 6 years ago

@twsouthwick would it better to wait for @PhenX to update this PR or start new session with link to this?

PhenX commented 6 years ago

Hello guys, I'm on vacation this week, I'll apply the changes next week :)

twsouthwick commented 6 years ago

Sounds like @PhenX is on this - sorry for the slow responses. I meant to tell @PhenX thanks for the fixes - sorry for the confusion. Have a good vacation and I'll be watching for any updates here so we can get it merged in.

twsouthwick commented 6 years ago

Looks like the test is still failing. Are you going to update the System.IO.Packaging required?

PhenX commented 6 years ago

@twsouthwick I could not find a way to update System.IO.Packaging by including a NuGet package. It doesn't work better. I think we would need to upgrade to netcoreapp2.1 (as you said in your first answser, it's a bug in NetCore 2.0), do you have any other suggestion ?

twsouthwick commented 6 years ago

Go ahead and target 2.1 for the tests - keep the library as .NET STandard 2.0, though

PhenX commented 6 years ago

Well, it still doesn't work, but it looks like @EricWhiteDev has had this problem once http://www.ericwhite.com/blog/forums/topic/system-io-ioexception-after-upgrade-to-openxml-sdk-2-6/

twsouthwick commented 6 years ago

Hmmm just so we can get this merged in for now, can you add a skip attribute to that test and open an issue?

PhenX commented 6 years ago

Just added the skip attribute, I'll open an issue once merged :)

twsouthwick commented 6 years ago

Cool. Thanks for getting this support! I'll merge this in, and we can deal with the outstanding issues in here later

PhenX commented 6 years ago

Great! I'll open the issue quickly. When do you think a new version of the nuget package can be released?

PhenX commented 6 years ago

@twsouthwick sorry to insist :blush:, when do you think a new version of the nuget package can be released?

twsouthwick commented 6 years ago

You can currently access the NuGet via the AppVeyor feed. We're working through some policy changes due to NuGet package signing for the Open XML SDK and will try to include this library in that so we can publish it to NuGet.

augb commented 6 years ago

You can currently access the NuGet via the AppVeyor feed. ...

Is this publicly available? If so, what is the feed URL?

twsouthwick commented 6 years ago

@augb It is public, but apparently wasn't documented. I've added it to the README

augb commented 6 years ago

Thank you @twsouthwick. There don't appear to be any packages: https://ci.appveyor.com/nuget/open-xml-powertools. When I use the dotnet cli command, I get a 500 - Internal Error:

dotnet add package OpenXmlPowerTools --source https://ci.appveyor.com/nuget/open-xml-powertools/api/v2/package
... elided ...
info :   InternalServerError https://ci.appveyor.com/nuget/open-xml-powertools/api/v2/package/FindPackagesById()?id='OpenXmlPowerTools'&semVerLevel=2.0.0 87ms
error: Failed to retrieve information about 'OpenXmlPowerTools' from remote source 'https://ci.appveyor.com/nuget/open-xml-powertools/api/v2/package/FindPackagesById()?id='OpenXmlPowerTools'&semVerLevel=2.0.0'.
error:   Response status code does not indicate success: 500 (Invalid NuGet feed request.).
twsouthwick commented 6 years ago

Hmmm that does appear to be the case. I'll look into that.

rmourato commented 6 years ago

Looking forward to an official Nuget package with these changes in!

PhenX commented 6 years ago

@twsouthwick Any new on this ? As already mentioned, a new version of the NuGet package would be really helpful for a lot of people.

PhenX commented 5 years ago

@EricWhiteDev @twsouthwick Sorry to bother you again, but I think the compatibility with NetStandard is an important point for the community, and having a NuGet package with the latest changes (including NetStandard support) is something a lot of people, including me, are waiting for. Thank you very much for your time taking this into account.