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

Add external DISM generations support. Fixes #5 & #25. #38

Merged
merged 7 commits into from
Jan 25, 2018
Merged

Add external DISM generations support. Fixes #5 & #25. #38

merged 7 commits into from
Jan 25, 2018

Conversation

Shidell
Copy link
Contributor

@Shidell Shidell commented Dec 27, 2017

Adds a "DismUtilities" class that adds support for external DISM libraries across DISM generations, which allows the user to (optionally) load external DISM libraries instead of defaulting to look for DISM on the local system.

@Shidell
Copy link
Contributor Author

Shidell commented Dec 27, 2017

@josemesona, I removed my previous fork and started over from scratch in an effort to clean up my branch (and all the changes therein), and better familiarize myself not only with Git, but also GitHub.

Now that I'm a bit more familiar, I've created a new fork to "start clean", so to speak, and re-added the Utilities class we've talked about in a separate branch.

Hopefully this is cleaner and easier to work with. I know you were interested in modifying the name (from "Utilities" to "DismUtilities"), and potentially integrating some of these changes into the Dism.Initialize() method. I've made the "DismUtilities" change, but haven't done anything else, as I also know this wrapper preserves the original native API mapping, and doesn't expand upon it.

I do like the fact that this managed API only wraps the native one, and leaves expanded functionality aside. Perhaps expanded functionality, such as this DISM generational library support, should remain in a "Utilities" (or other similar) classes, to preserve the clean mapping of the native API?

Ultimately, this decision is up to you. Feel free to modify this branch as you see fit to best integrate with this library. If you'd prefer I make the changes, just let me know how you'd like to see it implemented, and I can make another branch for you to review.

Once again, thanks for your patience while I'm getting up to speed with Git & GitHub.

@josemesona
Copy link
Contributor

I really like this functionality but again I'm just concerned that it moves away from being a pure wrapper.

If you make DismUtilities internal and add a new method to DismApi, I will gladly merge this.

public static partial class DismApi
{
    public static void InitializeEx(
        DismLogLevel logLevel,
        string logFilePath,
        string scratchDirectory,
        DismGeneration generation)
    {
      // Call DismUtilities.LoadDismGenerationLibrary(), throw an exception if it returns false
      // Call Initialize(logLevel, logFIlePath, scratchDirectory)
    }
}

Be sure to unload the library in Shutdown if the user specified calls InitializeEx. You can have all of the overloads if you want so that you can just call DismApi.Initailize(DismGeneration.Latest). You'll have to move DismGeneration out of DismUtilities and make it public which is fine.

This leaves DismUtilities in there and exposes its basic functionality as an input to an "Ex" method which is common in C++. What do you think?

@Shidell
Copy link
Contributor Author

Shidell commented Jan 12, 2018

I think that sounds like a good plan. I like the idea of using Ex methods, I just need to implement them.

I'll work to have something implemented shortly and update.

…expose initializing DISM with generational DISM library support.
@Shidell
Copy link
Contributor Author

Shidell commented Jan 15, 2018

I modified the branch as we discussed. A couple quick points:

  • I've added some "InitializeEx()" methods as we discussed which default to loading the latest DISM Generation library, assuming that's probably the best default setting. There is an overload where one can specify the library to load, but that introduces a problem: Now that the DismUtilities class is internal, a user cannot access the Properties in DismUtilities to quickly determine which DISM Generation libraries are available, if any.

  • I added a Property to the main DismApi class to hold the DISM Generation loaded (if one was loaded) to work two ways: First, a user can retrieve it to determine what version of the DISM Generation library they have loaded (if one is loaded) and two, if it is not set to "DismGeneration.NotFound", then this modification knows to unload the DISM Generation library in Shutdown().

I wanted to make these changes for further discussion before being prepared for merging; thus, I need to modify some comments (specifically in DismUtilities) to be more pertinent depending on what implementation we settle on.

I'm not thrilled by having a Property in DismApi to represent the DISM Generation that's loaded, because it feels like we're polluting the "pure" nature of your wrapper cleanly mapping the Win32 DISM API. On the other hand, I need to store whether or not a DISM Generation library was loaded, and this implementation also lets the user check what's in use, which (I think) is useful.

What are you thoughts, now with a more complete version of what we had discussed?

Copy link
Contributor

@josemesona josemesona left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I was out of town. Looks great!

/// <summary>
/// Represents the DISM Generational Library initialized for use with the DismApi Wrapper (via InitializeEx()). Returns the specific DismGeneration in use; otherwise, returns DismGeneration.NotFound.
/// </summary>
public static DismGeneration DismGenerationInitialized
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this internal, an autoproperty, and set a initial value so you can get rid of the dismGenerationInitialized field and setting it in Initialize

internal static DismGeneration DismGenerationInitialized { get; set; } = DismGeneration.None;

/// <exception cref="DismException">When a failure occurs.</exception>
public static void InitializeEx(DismLogLevel logLevel)
{
DismGeneration dismGeneration = DismUtilities.GetLatestDismGeneration();
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs to do nothing if its already been initialized right?

if(DismGenerationInitialized != DismGeneration.None)
{
    return;
}

Or maybe even throw, I'm not sure what Initialize will do if you accidentally call it twice...

…InitializeEx() functions and added a new Exception to handle calling InitializeEx() when a DISM Generation library is already loaded.
@Shidell
Copy link
Contributor Author

Shidell commented Jan 25, 2018

No problem -- everybody is busy. :)

Good call on the Automatic Property. I made that change, and cleaned up the InitializeEx() methods as well, so the overloads cascade instead of repeating the same logic. I also added an additional Exception handler, in the event the user does try to InitializeEx() after already loading a DISM Generation library and before freeing it (Shutdown()).

Any other thoughts or changes you can think of?

EDIT: I should note, if/when we're comfortable with the "DISM Generation" changes, I'll modify the DismUtilities comments to better reflect the changes we've agreed upon, and submit that as a change as well for review.

@josemesona
Copy link
Contributor

I've pushed a few commits, double check them and I'll merge!

@Shidell
Copy link
Contributor Author

Shidell commented Jan 25, 2018

Looks great! I like moving the expanded functionality in DismApiEx, good idea.

EDIT: If you're waiting on me to review or approve the changes, I looked, but I can't approve changes on my own pull request. (Still relatively new to GitHub.)

@josemesona
Copy link
Contributor

That's fine, I was just waiting for you to comment. Thanks for the contribution, I'll push a package.

@josemesona josemesona merged commit 2b8dd63 into jeffkl:master Jan 25, 2018
@josemesona
Copy link
Contributor

Package will be available at https://www.nuget.org/packages/Microsoft.Dism/1.6.446 in about 10 minutes (it is being "validated" by nuget.org)

@Shidell Shidell deleted the add-dism-utilities-support branch January 30, 2018 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants