GNOME Bugzilla – Bug 677038
Small UnattendedFile cleanups
Last modified: 2016-03-31 13:53:51 UTC
In preparation for the work I've done to automatically install the virtio/qxl guest drivers and the SPICE agent, here are some small patches to the UnattendedFile classes.
Created attachment 215184 [details] [review] Store hostname as an UnattendedInstaller property This allows to make the code setting the host name in unattended files much less convoluted. https://bugzilla.gnome.org/show_bug.cgi?id=676834
Created attachment 215185 [details] [review] Remove unused/unneeded UnattendedInstaller properties UnattendedInstaller::unattended_src_path is unused. UnattendedInstaller::unattended_dest_name is used by the FedoraInstaller class. Since the value of this property is set from a hardcoded string provided by the FedoraInstaller class, there is no need to go through this indirection.
Created attachment 215186 [details] [review] Use add_unattended_text_file for 'main' install script In unattended installer subclasses, the 'main' file (ks.cfg, winnt.sif, autounattend.xml) is not handed to UnattendedInstaller through its add_unattended_file method, but through extra args in its ::copy method, which then open code add_unattended_file. Moving this logic out of ::copy makes things simpler.
Created attachment 215187 [details] [review] Rename UnattendedInstaller::copy to ::from_media It's not a real copy constructor, and the naming is confusing because these classes will be copying files as well.
Created attachment 215188 [details] [review] Pass UnattendedFile to add_unattended_file This makes add_unattended_file usable with any class inheriting from UnattendedFile, not only with UnattendedTextFile instances.
Created attachment 215189 [details] [review] Turn the UnattendedFile iface into a real class It does a straight copy of the file to the floppy image. It can be subclassed to get the UnattendedAvatar or UnattendedTextFile behaviour. This will be used to copy drivers to Windows XP floppy image to be able to use viostor during WinXP installs.
Review of attachment 215184 [details] [review]: After thinking some more about this, I think the best option is the alternative you suggested: Instead of UnattendedTextFile getting a ref to UnattendedInstaller, it gets a function to convert unattended data. Then we can make use of lambda function to neither keep nor pass around vm/hostname.
Review of attachment 215185 [details] [review]: ACK!
Review of attachment 215186 [details] [review]: I had a reason to do it this way: There is at least one main unattendedtext file needed by every implementation and having this arguments makes the 'mandatory' aspect clear.
Review of attachment 215187 [details] [review]: ACK
Review of attachment 215188 [details] [review]: ACK
Review of attachment 215189 [details] [review]: I don't get this one. What you need to do that can't be done with interface but a class?
(In reply to comment #12) > Review of attachment 215189 [details] [review]: > > I don't get this one. What you need to do that can't be done with interface but > a class? I want to be able to do add_unattended_file (new UnattendedFile (this, "binaryfile.bin", "foo")); to copy an arbitrary file to the floppy image with no transformation.
(In reply to comment #9) > Review of attachment 215186 [details] [review]: > > I had a reason to do it this way: There is at least one main unattendedtext > file needed by every implementation and having this arguments makes the > 'mandatory' aspect clear. What about if I change UnattendedInstaller.copy prototype to UnattendedInstaller.copy (InstallerMedia media, UnattendedFile main_unattended, AvatarFormat?), would that work for you ?
(In reply to comment #7) > Review of attachment 215184 [details] [review]: > > After thinking some more about this, I think the best option is the alternative > you suggested: Instead of UnattendedTextFile getting a ref to > UnattendedInstaller, it gets a function to convert unattended data. Then we can > make use of lambda function to neither keep nor pass around vm/hostname. Would you mind doing this? As said earlier, I have no clue how to do that in Vala ;) If I'm not mistaken, the change you suggest would be an additional patch on top of attachment #215184 [details], right?
(In reply to comment #13) > (In reply to comment #12) > > Review of attachment 215189 [details] [review] [details]: > > > > I don't get this one. What you need to do that can't be done with interface but > > a class? > > I want to be able to do > add_unattended_file (new UnattendedFile (this, "binaryfile.bin", "foo")); > to copy an arbitrary file to the floppy image with no transformation. I get that part and I'm good with that. I was asking why UnattendedFile needs to become a class? (In reply to comment #14) > (In reply to comment #9) > > Review of attachment 215186 [details] [review] [details]: > > > > I had a reason to do it this way: There is at least one main unattendedtext > > file needed by every implementation and having this arguments makes the > > 'mandatory' aspect clear. > > What about if I change UnattendedInstaller.copy prototype to > UnattendedInstaller.copy (InstallerMedia media, UnattendedFile main_unattended, > AvatarFormat?), would that work for you ? Yeah, I was going to suggest that actually. :) (In reply to comment #15) > (In reply to comment #7) > > Review of attachment 215184 [details] [review] [details]: > > > > After thinking some more about this, I think the best option is the alternative > > you suggested: Instead of UnattendedTextFile getting a ref to > > UnattendedInstaller, it gets a function to convert unattended data. Then we can > > make use of lambda function to neither keep nor pass around vm/hostname. > > Would you mind doing this? As said earlier, I have no clue how to do that in > Vala ;) If I'm not mistaken, the change you suggest would be an additional > patch on top of attachment #215184 [details], right? https://live.gnome.org/Vala/Tutorial#Delegates But yeah, I can handle that once your other patches here are pushed.
(In reply to comment #16) > (In reply to comment #13) > > > > I want to be able to do > > add_unattended_file (new UnattendedFile (this, "binaryfile.bin", "foo")); > > to copy an arbitrary file to the floppy image with no transformation. > > I get that part and I'm good with that. I was asking why UnattendedFile needs > to become a class? > Sorry, I don't understand what you're getting at, I need a class to be able to instantiate an object representing a binary file. I'm missing something here in your question.
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #13) > > > > > > I want to be able to do > > > add_unattended_file (new UnattendedFile (this, "binaryfile.bin", "foo")); > > > to copy an arbitrary file to the floppy image with no transformation. > > > > I get that part and I'm good with that. I was asking why UnattendedFile needs > > to become a class? > > > > Sorry, I don't understand what you're getting at, I need a class to be able to > instantiate an object representing a binary file. I'm missing something here in > your question. Apparently I misread your comment and code, sorry! Based on our previous discussions, I assumed that you'll be creating a new UnattendedFile class/impl. for your drivers/binaries. I think that would be a more cleaner way. The copy implementation is the same for both implementations so i don't see why it should be repeated in every subclass.
Created attachment 215555 [details] [review] Store hostname as an UnattendedInstaller property This allows to make the code setting the host name in unattended files much less convoluted.
Created attachment 215556 [details] [review] Remove unused/unneeded UnattendedInstaller properties UnattendedInstaller::unattended_src_path is unused. UnattendedInstaller::unattended_dest_name is used by the FedoraInstaller class. Since the value of this property is set from a hardcoded string provided by the FedoraInstaller class, there is no need to go through this indirection.
Created attachment 215557 [details] [review] Pass UnattendedFile to UnattendedInstaller::copy Now that we have fairly generic UnattendedFile classes, it's better to use those to pass the main unattended file to UnattendedInstaller::copy instead of creating it by hand in the constructor.
Created attachment 215558 [details] [review] Rename UnattendedInstaller::copy to ::from_media It's not a real copy constructor, and the naming is confusing because these classes will be copying files as well.
Created attachment 215559 [details] [review] Pass UnattendedFile to add_unattended_file This makes add_unattended_file usable with any class inheriting from UnattendedFile, not only with UnattendedTextFile instances.
Created attachment 215560 [details] [review] Turn the UnattendedFile iface into a real class It does a straight copy of the file to the floppy image. It can be subclassed to get the UnattendedAvatar or UnattendedTextFile behaviour. This will be used to copy drivers to Windows XP floppy image to be able to use viostor during WinXP installs.
(In reply to comment #18) > Apparently I misread your comment and code, sorry! Based on our previous > discussions, I assumed that you'll be creating a new UnattendedFile class/impl. > for your drivers/binaries. I think that would be a more cleaner way. The copy > implementation is the same for both implementations so i don't see why it > should be repeated in every subclass. I haven't addressed this in the revised series I've attached. I think there are 2 issues here 1) should the last patch introduce an UnattendedRawCopyFile class which implements the UnattendedFile interface, or should this new class replace the UnattendedFile interface 2) code duplication (in UnattendedAvatarFile/UnattendedTextFile ?) For 1), adding a new class would be a bit overkill imo as it would be boilerplate only since it does not add anything over what is already in UnattendedFile For 2) I'll have to look at it And maybe there's another issue, namely 3) I didn't get what you meant ;)
Comment on attachment 215557 [details] [review] Pass UnattendedFile to UnattendedInstaller::copy >From af31e2438ff403d3ca0b1df3c7e30e0ffb9a9c0a Mon Sep 17 00:00:00 2001 >From: Christophe Fergeau <cfergeau@redhat.com> >Date: Thu, 24 May 2012 15:16:35 +0200 >Subject: [PATCH] Pass UnattendedFile to UnattendedInstaller::copy > >Now that we have fairly generic UnattendedFile classes, it's >better to use those to pass the main unattended file to >UnattendedInstaller::copy instead of creating it by hand >in the constructor. > >https://bugzilla.gnome.org/show_bug.cgi?id=677038 >--- > src/fedora-installer.vala | 5 +++-- > src/unattended-installer.vala | 10 +++++----- > src/win7-installer.vala | 6 +++--- > src/winxp-installer.vala | 5 +++-- > 4 files changed, 14 insertions(+), 12 deletions(-) > >diff --git a/src/fedora-installer.vala b/src/fedora-installer.vala >index 03d9f3f..9e0a85a 100644 >--- a/src/fedora-installer.vala >+++ b/src/fedora-installer.vala >@@ -30,9 +30,10 @@ private class Boxes.FedoraInstaller: UnattendedInstaller { > } > > public FedoraInstaller.copy (InstallerMedia media) throws GLib.Error { >- var source_path = get_unattended ("fedora.ks"); >+ string unattended_source = get_unattended ("fedora.ks"); >+ var unattended_file = new UnattendedTextFile(this, unattended_source, "ks.cfg"); >+ base.copy (media, unattended_file); > This patch is broken, but I'm not sure how I can get something that works. My problem is that 'this' is not initialized until base.copy returns, but I need it to create the unattended_file I'm passing to base.copy. Is there any way around this ?
(In reply to comment #25) > (In reply to comment #18) > > Apparently I misread your comment and code, sorry! Based on our previous > > discussions, I assumed that you'll be creating a new UnattendedFile class/impl. > > for your drivers/binaries. I think that would be a more cleaner way. The copy > > implementation is the same for both implementations so i don't see why it > > should be repeated in every subclass. > > I haven't addressed this in the revised series I've attached. I think there are > 2 issues here > 1) should the last patch introduce an UnattendedRawCopyFile class which > implements the UnattendedFile interface, or should this new class replace the > UnattendedFile interface > 2) code duplication (in UnattendedAvatarFile/UnattendedTextFile ?) > > For 1), adding a new class would be a bit overkill imo as it would be > boilerplate only since it does not add anything over what is already in > UnattendedFile If you could address issue#2, I can agree with you on #1. Maybe its as simple as moving 'copy' implementation into base UnattendedFile Class. > For 2) I'll have to look at it > And maybe there's another issue, namely 3) I didn't get what you meant ;) If you could tell me which part you didn't understand, I can try to explain better.
(In reply to comment #26) > (From update of attachment 215557 [details] [review]) > >From af31e2438ff403d3ca0b1df3c7e30e0ffb9a9c0a Mon Sep 17 00:00:00 2001 > >From: Christophe Fergeau <cfergeau@redhat.com> > >Date: Thu, 24 May 2012 15:16:35 +0200 > >Subject: [PATCH] Pass UnattendedFile to UnattendedInstaller::copy > > > >Now that we have fairly generic UnattendedFile classes, it's > >better to use those to pass the main unattended file to > >UnattendedInstaller::copy instead of creating it by hand > >in the constructor. > > > >https://bugzilla.gnome.org/show_bug.cgi?id=677038 > >--- > > src/fedora-installer.vala | 5 +++-- > > src/unattended-installer.vala | 10 +++++----- > > src/win7-installer.vala | 6 +++--- > > src/winxp-installer.vala | 5 +++-- > > 4 files changed, 14 insertions(+), 12 deletions(-) > > > >diff --git a/src/fedora-installer.vala b/src/fedora-installer.vala > >index 03d9f3f..9e0a85a 100644 > >--- a/src/fedora-installer.vala > >+++ b/src/fedora-installer.vala > >@@ -30,9 +30,10 @@ private class Boxes.FedoraInstaller: UnattendedInstaller { > > } > > > > public FedoraInstaller.copy (InstallerMedia media) throws GLib.Error { > >- var source_path = get_unattended ("fedora.ks"); > >+ string unattended_source = get_unattended ("fedora.ks"); > >+ var unattended_file = new UnattendedTextFile(this, unattended_source, "ks.cfg"); > >+ base.copy (media, unattended_file); > > > > This patch is broken, but I'm not sure how I can get something that works. My > problem is that 'this' is not initialized until base.copy returns, but I need > it to create the unattended_file I'm passing to base.copy. Is there any way > around this ? Now I recall that I already made this change locally and gave-up on this after I realized the issue with it. Simplest work-around would be for UnattendedTextFile.installer to be nullable, which you can set later. IMHO, thats really not worth it and I'd keep this the way it is but its up to you.
(In reply to comment #27) > > If you could address issue#2, I can agree with you on #1. Maybe its as simple > as moving 'copy' implementation into base UnattendedFile Class. > I'll look into #2 then. I tend to consider that having empty vfuncs in base classes that will be overriden by children classes is an indication of an imperfect class hierarchy, that's why I'm not a big fan of having copy in the base class. I'll see if I can come up with something different.
(In reply to comment #28) > Now I recall that I already made this change locally and gave-up on this after > I realized the issue with it. Simplest work-around would be for > UnattendedTextFile.installer to be nullable, which you can set later. IMHO, > thats really not worth it and I'd keep this the way it is but its up to you. Agreed, though I'm very bored with changing this interface again and again since it generates quite some conflicts ;) An alternative is the UnattendedFile::set_unattended_recipe() to be called from the various ::from_media function which I proposed somewhere in this bug.
(In reply to comment #29) > (In reply to comment #27) > > > > If you could address issue#2, I can agree with you on #1. Maybe its as simple > > as moving 'copy' implementation into base UnattendedFile Class. > > > > I'll look into #2 then. I tend to consider that having empty vfuncs in base > classes that will be overriden by children classes is an indication of an > imperfect class hierarchy, I don't agree, its just an indication that not all subclasses will need to do anything on that stage.
Created attachment 217365 [details] [review] Factor common code in UnattendedFile children Introduce an abstract class to factor the common code between UnattendedTextFile and UnattendedAvatarFile, which is "copy the data to a temporary file while applying a filter on it, call the code to copy to the floppy image, and delete the temporary file"
Created attachment 217366 [details] [review] UnattendedTransformFile: robustify temp file removal
(In reply to comment #27) > If you could address issue#2, I can agree with you on #1. Maybe its as simple > as moving 'copy' implementation into base UnattendedFile Class. Patch in comment #32 comes as a followup to the series in this bug and addresses #2. Patch in comment #33 is an additional improvement.
Created attachment 217368 [details] [review] Store hostname as an UnattendedInstaller property This allows to make the code setting the host name in unattended files much less convoluted.
(In reply to comment #35) > Created an attachment (id=217368) [details] [review] > Store hostname as an UnattendedInstaller property > > This allows to make the code setting the host name in unattended > files much less convoluted. This one needed rediff'ing against master. I can repost the whole series if needed
Review of attachment 217368 [details] [review]: So you decided not to learn about delegates in Vala after all. :) As agreed I'll rework this one after other patches are in.
I'll repost the series with whatever patch was not working dropped (reviewing patch series in bugzilla just sucks)
Created attachment 217494 [details] [review] Store hostname as an UnattendedInstaller property This allows to make the code setting the host name in unattended files much less convoluted.
Created attachment 217495 [details] [review] Remove unused/unneeded UnattendedInstaller properties UnattendedInstaller::unattended_src_path is unused. UnattendedInstaller::unattended_dest_name is used by the FedoraInstaller class. Since the value of this property is set from a hardcoded string provided by the FedoraInstaller class, there is no need to go through this indirection.
Created attachment 217496 [details] [review] Rename UnattendedInstaller::copy to ::from_media It's not a real copy constructor, and the naming is confusing because these classes will be copying files as well.
Created attachment 217497 [details] [review] Pass UnattendedFile to add_unattended_file This makes add_unattended_file usable with any class inheriting from UnattendedFile, not only with UnattendedTextFile instances.
Created attachment 217498 [details] [review] Turn the UnattendedFile iface into a real class It does a straight copy of the file to the floppy image. It can be subclassed to get the UnattendedAvatar or UnattendedTextFile behaviour. This will be used to copy drivers to Windows XP floppy image to be able to use viostor during WinXP installs.
Created attachment 217499 [details] [review] Factor common code in UnattendedFile children Introduce an abstract class to factor the common code between UnattendedTextFile and UnattendedAvatarFile, which is "copy the data to a temporary file while applying a filter on it, call the code to copy to the floppy image, and delete the temporary file"
Created attachment 217501 [details] [review] UnattendedTransformFile: robustify temp file removal
Review of attachment 217495 [details] [review]: ACK
Review of attachment 217496 [details] [review]: ACK
Review of attachment 217497 [details] [review]: ACK
I'd need a review of "Store hostname as an UnattendedInstaller property" before I can push the 3 patches that have already been ACK'ed
(In reply to comment #49) > I'd need a review of "Store hostname as an UnattendedInstaller property" before > I can push the 3 patches that have already been ACK'ed But thats the one that I need to provide a replacement for, as agreed in comment#16. I'll look into that soon..
Ah I see this change as an additional commit that could be done after this series (or along with "Turn the UnattendedFile iface into a real class "), but I understand your position as well, I just didn't think about this. I didn't mean to put some pressure on you doing this ). It wasn't clear to me that this was a must-have that must be done before the patch goes in :-/
(In reply to comment #51) > Ah I see this change as an additional commit that could be done after this > series (or along with "Turn the UnattendedFile iface into a real class "), but > I understand your position as well, I just didn't think about this. I didn't > mean to put some pressure on you doing this ). It wasn't clear to me that this > was a must-have that must be done before the patch goes in :-/ Apart from "Turn the UnattendedFile iface into a real class", the rest really could go in for now and I can change things later.
Review of attachment 217494 [details] [review]: AFAIK this could simply be dropped with any (major) merge conflicts so I suggest you drop this for now and I'll attempt to solve this with delegates. If I fail, we can go for this one. :)
Review of attachment 217499 [details] [review]: As for the main motivation of this patch, AFAICT, its only needed after your other patches on this bug (i-e there isn't much that UnattendedTextFile and UnattendedAvatarFile duplicate in git master). ::: src/unattended-installer.vala @@ -350,3 +350,3 @@ } -private class Boxes.UnattendedTextFile: Boxes.UnattendedFile { +private abstract class Boxes.UnattendedTransformFile : Boxes.UnattendedFile { Why introduce another class? Can't UnattendedFile do everything UnattendedTransformFile do? @@ -360,2 +359,2 @@ public override async void copy (Cancellable? cancellable) throws GLib.Error { - var unattended_tmp = yield create (cancellable); + var unattended_tmp = yield transform_to_file (cancellable); this name sounds even more wrong to me as it gives the impression that we have something not a file and we transform it into one. From the POV of the base class, the file is being created by subclasses before its copied (most/all subclasses will create it by transforming a template file but thats no relavent in the base class).
Review of attachment 217501 [details] [review]: Otherwise good. ::: src/unattended-installer.vala @@ -364,3 +364,3 @@ - debug ("Deleting temporary file '%s'", unattended_tmp.get_path ()); - unattended_tmp.delete (cancellable); - debug ("Deleted temporary file '%s'", unattended_tmp.get_path ()); + yield copy_internal (unattended_tmp.get_path (), cancellable); + } finally { + try { I'd just call delete_file() from util.vala here.
Review of attachment 217498 [details] [review]: I still don't know/understand the reason you believe that you can't use interface for your goals and hence need this change.
Review of attachment 217498 [details] [review]: I don't see the point of pretending something is an interface while it's actually a base class. I could implement the interface to get an UnattendedRawFile, but it would just be empty. And hardcoding into the interface a behaviour some child class will need, but some others won't is just wrong.
(In reply to comment #53) > Review of attachment 217494 [details] [review]: > > AFAIK this could simply be dropped with any (major) merge conflicts so I > suggest you drop this for now and I'll attempt to solve this with delegates. If > I fail, we can go for this one. :) I'd rather get this in and remove this ugly passing of hostname everywhere. This doesn't cause major merge conflicts, but after having resolved these a dozen times already, it's getting a bit tiring.
(In reply to comment #54) > -private class Boxes.UnattendedTextFile: Boxes.UnattendedFile { > +private abstract class Boxes.UnattendedTransformFile : Boxes.UnattendedFile { > > Why introduce another class? Can't UnattendedFile do everything > UnattendedTransformFile do? As always, the less the base class knows about its children behaviour, the better. > > @@ -360,2 +359,2 @@ > public override async void copy (Cancellable? cancellable) throws > GLib.Error { > - var unattended_tmp = yield create (cancellable); > + var unattended_tmp = yield transform_to_file (cancellable); > > this name sounds even more wrong to me as it gives the impression that we have > something not a file and we transform it into one. From the POV of the base > class, the file is being created by subclasses before its copied (most/all > subclasses will create it by transforming a template file but thats no relavent > in the base class). I'm not very attached to the naming, I don't like transform very much either, I tried 'Filter' as well, dunno if that would work better for you?
In short, I'd like to get the first 4 patches in ("Store hostname as an UnattendedInstaller property" + the 3 that are marked as "commit_now"), this would remove a big source of conflicts when rebasing, then I can work some more on the UnattendedFile stuff. And I'm ok with rebasing the patches from bug #678900 on top of that.
Review of attachment 217494 [details] [review]: imho, this should have be made this way from the beginning, and I don't see why this patch shouldn't be applied already. ack from me
sorry for the spam, bz is kinda broken here.
Comment on attachment 217494 [details] [review] Store hostname as an UnattendedInstaller property OK, lets do it this way then!
(In reply to comment #59) > (In reply to comment #54) > > > -private class Boxes.UnattendedTextFile: Boxes.UnattendedFile { > > +private abstract class Boxes.UnattendedTransformFile : Boxes.UnattendedFile { > > > > Why introduce another class? Can't UnattendedFile do everything > > UnattendedTransformFile do? > > As always, the less the base class knows about its children behaviour, the > better. Thats true for every class, not just the base classes. > > > > @@ -360,2 +359,2 @@ > > public override async void copy (Cancellable? cancellable) throws > > GLib.Error { > > - var unattended_tmp = yield create (cancellable); > > + var unattended_tmp = yield transform_to_file (cancellable); > > > > this name sounds even more wrong to me as it gives the impression that we have > > something not a file and we transform it into one. From the POV of the base > > class, the file is being created by subclasses before its copied (most/all > > subclasses will create it by transforming a template file but thats no relavent > > in the base class). > > I'm not very attached to the naming, I don't like transform very much either, I > tried 'Filter' as well, dunno if that would work better for you? No, I was trying to say that there isn't any need to point out that this function needs to do anything other than create the file. Subclasses can transform or filter or whatever they need to do to get the file created but from the pov of UnattendedFile, this function just does what it says. I'm assuming here that we don't need the intermediate class you added.
(In reply to comment #60) > In short, I'd like to get the first 4 patches in ("Store hostname as an > UnattendedInstaller property" + the 3 that are marked as "commit_now"), this > would remove a big source of conflicts when rebasing, then I can work some more > on the UnattendedFile stuff. And I'm ok with rebasing the patches from bug > #678900 on top of that. Now all 4 of them are ready for merge. :) I'll also rebase patches in bug#678900 after you merge these.
(In reply to comment #66) > No, I was trying to say that there isn't any need to point out that this > function needs to do anything other than create the file. Subclasses can > transform or filter or whatever they need to do to get the file created but > from the pov of UnattendedFile, this function just does what it says. I'm > assuming here that we don't need the intermediate class you added. Seems we just have different point of views. You consider that creating a temporary file for the copy is what all classes implementing UnattendedFile should do, and that classes which don't need an intermediate file are the odd case. My point of view is that UnattendedFile is about copying files to a floppy image, but doing it through a temporary file is a specialized behaviour belonging in a child class, not something that belongs in the generic UnattendedFile interface. I don't care a lot about that, I'll keep the useless specialized code in the interface if that what it takes to get that code in.
(In reply to comment #68) > (In reply to comment #66) > > > No, I was trying to say that there isn't any need to point out that this > > function needs to do anything other than create the file. Subclasses can > > transform or filter or whatever they need to do to get the file created but > > from the pov of UnattendedFile, this function just does what it says. I'm > > assuming here that we don't need the intermediate class you added. > > Seems we just have different point of views. You consider that creating a > temporary file for the copy is what all classes implementing UnattendedFile > should do, and that classes which don't need an intermediate file are the odd > case. > My point of view is that UnattendedFile is about copying files to a floppy > image, but doing it through a temporary file is a specialized behaviour > belonging in a child class, not something that belongs in the generic > UnattendedFile interface. You are correct, except that I don't assume the file to be temporary: A subclass could just as well return the GLib.File pointing to an original file. 'create' might be a wrong term in that case but we can rename this if there arises such a case.
Attachment 217494 [details] pushed as 21a8c2b - Store hostname as an UnattendedInstaller property Attachment 217495 [details] pushed as 1114071 - Remove unused/unneeded UnattendedInstaller properties Attachment 217496 [details] pushed as f1043b8 - Rename UnattendedInstaller::copy to ::from_media Attachment 217497 [details] pushed as a88b7b8 - Pass UnattendedFile to add_unattended_file
Created attachment 218076 [details] [review] Rework UnattendedFile interface Don't hardcode in the base interface that UnattendedFile subclasses will use a temporary file. Given that the various UnattendedInstaller instances stick around forever, we have to make sure UnattendedInstaller::unattended_files is emptied once the files are copied so that their destructors run to do the cleanup.
Created attachment 218077 [details] [review] Add UnattendedRawFile class
Another attempt a adding a class to copy a file with no changes. I haven't run this yet. And I'm still unconvinced the interface is needed ;)
(In reply to comment #73) > Another attempt a adding a class to copy a file with no changes. I haven't run > this yet. And I'm still unconvinced the interface is needed ;) Just tested winxp and fedora unattended, both work.
(In reply to comment #74) > Just tested winxp and fedora unattended, both work. So does win7 unattended
Review of attachment 218076 [details] [review]: Looks good otherwise. ::: src/unattended-installer.vala @@ -134,2 +134,3 @@ foreach (var unattended_file in unattended_files) yield unattended_file.copy (cancellable); + unattended_files = null; We would want to do this also when there is an error from any of the unattended_file.copy above?
Review of attachment 218077 [details] [review]: Looks good but this should go along with the patch(es) that uses this class. Not saying it should be squashed into that.
(In reply to comment #76) > We would want to do this also when there is an error from any of the > unattended_file.copy above? Yes, good catch, I'll move that to a finally clause. (In reply to comment #77) > Review of attachment 218077 [details] [review]: > > Looks good but this should go along with the patch(es) that uses this class. > Not saying it should be squashed into that. Fair enough. It's self contained now so should be no big deal to rebase anyway.
Created attachment 218093 [details] [review] Rework UnattendedFile interface Don't hardcode in the base interface that UnattendedFile subclasses will use a temporary file. Given that the various UnattendedInstaller instances stick around forever, we have to make sure UnattendedInstaller::unattended_files is emptied once the files are copied so that their destructors run to do the cleanup.
Review of attachment 218093 [details] [review]: ACK ::: src/unattended-installer.vala @@ -363,3 +364,3 @@ } - protected async File create (Cancellable? cancellable) throws GLib.Error { + ~UnattendedTextFile () { BTW, since you already convinced me on that, I take it that you'll be submitting a new version of the 'UnattendedTransformFile' patch and move this there? Not a lot of code so don't mind either way.
(In reply to comment #80) > Review of attachment 218093 [details] [review]: > > ACK > > ::: src/unattended-installer.vala > @@ -363,3 +364,3 @@ > } > > - protected async File create (Cancellable? cancellable) throws GLib.Error > { > + ~UnattendedTextFile () { > > BTW, since you already convinced me on that, I take it that you'll be > submitting a new version of the 'UnattendedTransformFile' patch and move this > there? Not a lot of code so don't mind either way. The Avatar and Text classes will probably get merged into a single one when we switch to using a delegate there, so I wasn't planning to do more work on this intermediate class, especially since it's only one or 2 duplicated lines now.
Created attachment 218417 [details] [review] Rework UnattendedFile interface Don't hardcode in the base interface that UnattendedFile subclasses will use a temporary file. Given that the various UnattendedInstaller instances stick around forever, we have to make sure UnattendedInstaller::unattended_files is emptied once the files are copied so that their destructors run to do the cleanup.
Rediff'ed patch against master which I'll push in a few days if there are no objections (previous one was ack'ed).
(In reply to comment #81) > (In reply to comment #80) > > Review of attachment 218093 [details] [review] [details]: > > > > ACK > > > > ::: src/unattended-installer.vala > > @@ -363,3 +364,3 @@ > > } > > > > - protected async File create (Cancellable? cancellable) throws GLib.Error > > { > > + ~UnattendedTextFile () { > > > > BTW, since you already convinced me on that, I take it that you'll be > > submitting a new version of the 'UnattendedTransformFile' patch and move this > > there? Not a lot of code so don't mind either way. > > The Avatar and Text classes will probably get merged into a single one when we > switch to using a delegate there, so I wasn't planning to do more work on this > intermediate class, especially since it's only one or 2 duplicated lines now. Why and how would they be merged? If you are referring to our discussion about use of delegates in this bug, that was about passing UnattendedInstaller.fill_unattended_data to UnattendedFile classes rather than UnattendedInstaller instance. The main rationale for that was the hostname passing around, which isn't the case anymore.
(In reply to comment #84) > Why and how would they be merged? If you are referring to our discussion about > use of delegates in this bug, that was about passing > UnattendedInstaller.fill_unattended_data to UnattendedFile classes rather than > UnattendedInstaller instance. The main rationale for that was the hostname > passing around, which isn't the case anymore. Nope, the main rationale for this is untying UnattendedFile from UnattendedInstaller and having all the UnattendedFile classes be standalone, so it still make sense to do that one day. With regards to this patch, should I take it that you want that small duplication removed? If so, what is your suggestion? adding an intermediate class to handle that (whose name has to be found ;) ?
(In reply to comment #85) > With regards to this patch, should I take it that you want that small > duplication removed? If so, what is your suggestion? adding an intermediate > class to handle that (whose name has to be found ;) ? Yes and as I said later on IRC, I'm ok with that class and the name you chose for it. :)
What was it ? Tranform ? I thought you hated it? or did we find something else?
(In reply to comment #87) > What was it ? Tranform ? I thought you hated it? or did we find something else? After so much discussion on this bug, I'm finding it hard to keep track or remember everything. IIRC it was that I had misunderstood something.
Created attachment 221552 [details] [review] Rework UnattendedFile interface Don't hardcode in the base interface that UnattendedFile subclasses will use a temporary file. Given that the various UnattendedInstaller instances stick around forever, we have to make sure UnattendedInstaller::unattended_files is emptied once the files are copied so that their destructors run to do the cleanup. https://bugzilla.gnome.org/show_bug.cgi?id=677038 https://bugzilla.gnome.org/show_bug.cgi?id=679752
(In reply to comment #89) > Created an attachment (id=221552) [details] [review] > Rework UnattendedFile interface I realized I had never attached the reworked version of this patch rebased against master. It's needed on top of master before the viostor series from bug#679752 can be applied. I think all the other patches in this bug are obsolete.
Review of attachment 221552 [details] [review]: Apart from this duplication, looks right! ::: src/unattended-installer.vala @@ -378,2 +377,3 @@ protected UnattendedInstaller installer { get; set; } + private File unattended_tmp; Since there is no destructors in interfaces, this would be a good reason to convert UnattendedFile into a class and have this deletion there?
Review of attachment 221552 [details] [review]: I've just pushed this patch as is, I only remembered about your comment after seeing it again now, sorry about that :-/