Just opened bug 4609 against v3.9

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Just opened bug 4609 against v3.9

Blair Murri-3
http://wixtoolset.org/issues/4609/ is the issue I mentioned in the previous weekly meeting. It isn't as bad as we first thought, but it does cripple at least one use case (setting a default value for a hidden variable) and (at least in the non-elevated UI engine instance, I did not look in the elevated apply engine instance) it actually stores the plaintext version of its values, not the encrypted one (there is no case where they are stored encrypted).
 
If you want I could post a pull request against wixtoolset/wix3/master branch with my proposed fix, but I want to have some of you discuss whether you think this is a show stopper or not (my current client can live with the limitations for now).
 
Blair

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

John Cooper-2

That’s probably why I haven’t seen it.  All of our cases are elevated.

 

--

John Merryweather Cooper

Senior Software Engineer | Enterprise Service Applications | Continuing Development

Jack Henry & Associates, Inc.® | Lenexa, KS  66214 | Ext:  431050 |[hidden email]

 

 

 

From: Blair Murri [mailto:[hidden email]]
Sent: Monday, November 24, 2014 11:46 PM
To: [hidden email]
Subject: [WiX-devs] Just opened bug 4609 against v3.9

 

http://wixtoolset.org/issues/4609/ is the issue I mentioned in the previous weekly meeting. It isn't as bad as we first thought, but it does cripple at least one use case (setting a default value for a hidden variable) and (at least in the non-elevated UI engine instance, I did not look in the elevated apply engine instance) it actually stores the plaintext version of its values, not the encrypted one (there is no case where they are stored encrypted).
 
If you want I could post a pull request against wixtoolset/wix3/master branch with my proposed fix, but I want to have some of you discuss whether you think this is a show stopper or not (my current client can live with the limitations for now).
 
Blair

NOTICE: This electronic mail message and any files transmitted with it are intended
exclusively for the individual or entity to which it is addressed. The message,
together with any attachment, may contain confidential and/or privileged information.
Any unauthorized review, use, printing, saving, copying, disclosure or distribution
is strictly prohibited. If you have received this message in error, please
immediately advise the sender by reply email and delete all copies.


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

SeanHall
That's pretty embarrassing, I did all that work to try to encrypt the value and it doesn't actually stay encrypted.  I agree with your analysis and proposed fix.

I don't know what the process would be, but if you already have the change ready then I would go ahead and submit a pull request to the develop branch for wix3.

On Tue, Nov 25, 2014 at 8:16 AM, John Cooper <[hidden email]> wrote:

That’s probably why I haven’t seen it.  All of our cases are elevated.

 

--

John Merryweather Cooper

Senior Software Engineer | Enterprise Service Applications | Continuing Development

Jack Henry & Associates, Inc.® | Lenexa, KS  66214 | Ext:  431050 |[hidden email]

 

 

 

From: Blair Murri [mailto:[hidden email]]
Sent: Monday, November 24, 2014 11:46 PM
To: [hidden email]
Subject: [WiX-devs] Just opened bug 4609 against v3.9

 

http://wixtoolset.org/issues/4609/ is the issue I mentioned in the previous weekly meeting. It isn't as bad as we first thought, but it does cripple at least one use case (setting a default value for a hidden variable) and (at least in the non-elevated UI engine instance, I did not look in the elevated apply engine instance) it actually stores the plaintext version of its values, not the encrypted one (there is no case where they are stored encrypted).
 
If you want I could post a pull request against wixtoolset/wix3/master branch with my proposed fix, but I want to have some of you discuss whether you think this is a show stopper or not (my current client can live with the limitations for now).
 
Blair

NOTICE: This electronic mail message and any files transmitted with it are intended
exclusively for the individual or entity to which it is addressed. The message,
together with any attachment, may contain confidential and/or privileged information.
Any unauthorized review, use, printing, saving, copying, disclosure or distribution
is strictly prohibited. If you have received this message in error, please
immediately advise the sender by reply email and delete all copies.


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs



------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Rob Mensching-7

Yes, submit the pull request against develop. We’ll figure out how to put it in master later if desired.

 

Where exactly is the fix right now?

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 6:36 AM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

That's pretty embarrassing, I did all that work to try to encrypt the value and it doesn't actually stay encrypted.  I agree with your analysis and proposed fix.

 

I don't know what the process would be, but if you already have the change ready then I would go ahead and submit a pull request to the develop branch for wix3.

 

On Tue, Nov 25, 2014 at 8:16 AM, John Cooper <[hidden email]> wrote:

That’s probably why I haven’t seen it.  All of our cases are elevated.

 

--

John Merryweather Cooper

Senior Software Engineer | Enterprise Service Applications | Continuing Development

Jack Henry & Associates, Inc.® | Lenexa, KS  66214 | Ext:  431050 |[hidden email]

 

 

 

From: Blair Murri [mailto:[hidden email]]
Sent: Monday, November 24, 2014 11:46 PM
To: [hidden email]
Subject: [WiX-devs] Just opened bug 4609 against v3.9

 

http://wixtoolset.org/issues/4609/ is the issue I mentioned in the previous weekly meeting. It isn't as bad as we first thought, but it does cripple at least one use case (setting a default value for a hidden variable) and (at least in the non-elevated UI engine instance, I did not look in the elevated apply engine instance) it actually stores the plaintext version of its values, not the encrypted one (there is no case where they are stored encrypted).
 
If you want I could post a pull request against wixtoolset/wix3/master branch with my proposed fix, but I want to have some of you discuss whether you think this is a show stopper or not (my current client can live with the limitations for now).
 
Blair

NOTICE: This electronic mail message and any files transmitted with it are intended
exclusively for the individual or entity to which it is addressed. The message,
together with any attachment, may contain confidential and/or privileged information.
Any unauthorized review, use, printing, saving, copying, disclosure or distribution
is strictly prohibited. If you have received this message in error, please
immediately advise the sender by reply email and delete all copies.


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Bob Arnson-6
In reply to this post by Blair Murri-3
On 25-Nov-14 00:46, Blair Murri wrote:
If you want I could post a pull request against wixtoolset/wix3/master branch with my proposed fix, but I want to have some of you discuss whether you think this is a show stopper or not (my current client can live with the limitations for now).
Unfortunately, but not a ship-stopper. If you have a fix, please make it against develop; this would obviously be a candidate for pulling into a servicing release if we did one for v3.9.
-- 
sig://boB
http://joyofsetup.com/

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Rob Mensching-7
In reply to this post by SeanHall

It’s okay. I missed in code review as well.  Anyway, we started digging into this at FireGiant and I expect we’ll have a pull request against WiX v3.10 very shortly.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 6:36 AM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

That's pretty embarrassing, I did all that work to try to encrypt the value and it doesn't actually stay encrypted.  I agree with your analysis and proposed fix.

 

I don't know what the process would be, but if you already have the change ready then I would go ahead and submit a pull request to the develop branch for wix3.

 

On Tue, Nov 25, 2014 at 8:16 AM, John Cooper <[hidden email]> wrote:

That’s probably why I haven’t seen it.  All of our cases are elevated.

 

--

John Merryweather Cooper

Senior Software Engineer | Enterprise Service Applications | Continuing Development

Jack Henry & Associates, Inc.® | Lenexa, KS  66214 | Ext:  431050 |[hidden email]

 

 

 

From: Blair Murri [mailto:[hidden email]]
Sent: Monday, November 24, 2014 11:46 PM
To: [hidden email]
Subject: [WiX-devs] Just opened bug 4609 against v3.9

 

http://wixtoolset.org/issues/4609/ is the issue I mentioned in the previous weekly meeting. It isn't as bad as we first thought, but it does cripple at least one use case (setting a default value for a hidden variable) and (at least in the non-elevated UI engine instance, I did not look in the elevated apply engine instance) it actually stores the plaintext version of its values, not the encrypted one (there is no case where they are stored encrypted).
 
If you want I could post a pull request against wixtoolset/wix3/master branch with my proposed fix, but I want to have some of you discuss whether you think this is a show stopper or not (my current client can live with the limitations for now).
 
Blair

NOTICE: This electronic mail message and any files transmitted with it are intended
exclusively for the individual or entity to which it is addressed. The message,
together with any attachment, may contain confidential and/or privileged information.
Any unauthorized review, use, printing, saving, copying, disclosure or distribution
is strictly prohibited. If you have received this message in error, please
immediately advise the sender by reply email and delete all copies.


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

SeanHall

On Tue, Nov 25, 2014 at 1:37 PM, Rob Mensching <[hidden email]> wrote:

It’s okay. I missed in code review as well.  Anyway, we started digging into this at FireGiant and I expect we’ll have a pull request against WiX v3.10 very shortly.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 6:36 AM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

That's pretty embarrassing, I did all that work to try to encrypt the value and it doesn't actually stay encrypted.  I agree with your analysis and proposed fix.

 

I don't know what the process would be, but if you already have the change ready then I would go ahead and submit a pull request to the develop branch for wix3.

 

On Tue, Nov 25, 2014 at 8:16 AM, John Cooper <[hidden email]> wrote:

That’s probably why I haven’t seen it.  All of our cases are elevated.

 

--

John Merryweather Cooper

Senior Software Engineer | Enterprise Service Applications | Continuing Development

Jack Henry & Associates, Inc.® | Lenexa, KS  66214 | Ext:  431050 |[hidden email]

 

 

 

From: Blair Murri [mailto:[hidden email]]
Sent: Monday, November 24, 2014 11:46 PM
To: [hidden email]
Subject: [WiX-devs] Just opened bug 4609 against v3.9

 

http://wixtoolset.org/issues/4609/ is the issue I mentioned in the previous weekly meeting. It isn't as bad as we first thought, but it does cripple at least one use case (setting a default value for a hidden variable) and (at least in the non-elevated UI engine instance, I did not look in the elevated apply engine instance) it actually stores the plaintext version of its values, not the encrypted one (there is no case where they are stored encrypted).
 
If you want I could post a pull request against wixtoolset/wix3/master branch with my proposed fix, but I want to have some of you discuss whether you think this is a show stopper or not (my current client can live with the limitations for now).
 
Blair

NOTICE: This electronic mail message and any files transmitted with it are intended
exclusively for the individual or entity to which it is addressed. The message,
together with any attachment, may contain confidential and/or privileged information.
Any unauthorized review, use, printing, saving, copying, disclosure or distribution
is strictly prohibited. If you have received this message in error, please
immediately advise the sender by reply email and delete all copies.


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs



------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Rob Mensching-7

Breaking change to BVariantCopy(). What if we always use the encryption state of Source and always nuke Target’s encryption state (like you are on variant.cpp:271)? Then we just need to get the Source state set correctly before copying.

 

I think that makes a lot of sense anyway, right?  Target encryption state matches Source encryption state on success.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 11:47 AM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

 

On Tue, Nov 25, 2014 at 1:37 PM, Rob Mensching <[hidden email]> wrote:

It’s okay. I missed in code review as well.  Anyway, we started digging into this at FireGiant and I expect we’ll have a pull request against WiX v3.10 very shortly.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 6:36 AM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

That's pretty embarrassing, I did all that work to try to encrypt the value and it doesn't actually stay encrypted.  I agree with your analysis and proposed fix.

 

I don't know what the process would be, but if you already have the change ready then I would go ahead and submit a pull request to the develop branch for wix3.

 

On Tue, Nov 25, 2014 at 8:16 AM, John Cooper <[hidden email]> wrote:

That’s probably why I haven’t seen it.  All of our cases are elevated.

 

--

John Merryweather Cooper

Senior Software Engineer | Enterprise Service Applications | Continuing Development

Jack Henry & Associates, Inc.® | Lenexa, KS  66214 | Ext:  431050 |[hidden email]

 

 

 

From: Blair Murri [mailto:[hidden email]]
Sent: Monday, November 24, 2014 11:46 PM
To: [hidden email]
Subject: [WiX-devs] Just opened bug 4609 against v3.9

 

http://wixtoolset.org/issues/4609/ is the issue I mentioned in the previous weekly meeting. It isn't as bad as we first thought, but it does cripple at least one use case (setting a default value for a hidden variable) and (at least in the non-elevated UI engine instance, I did not look in the elevated apply engine instance) it actually stores the plaintext version of its values, not the encrypted one (there is no case where they are stored encrypted).
 
If you want I could post a pull request against wixtoolset/wix3/master branch with my proposed fix, but I want to have some of you discuss whether you think this is a show stopper or not (my current client can live with the limitations for now).
 
Blair

NOTICE: This electronic mail message and any files transmitted with it are intended
exclusively for the individual or entity to which it is addressed. The message,
together with any attachment, may contain confidential and/or privileged information.
Any unauthorized review, use, printing, saving, copying, disclosure or distribution
is strictly prohibited. If you have received this message in error, please
immediately advise the sender by reply email and delete all copies.


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

SeanHall
That works, too.  That just means at least one additional encryption/decryption cycle, which I was trying to avoid.

On Tue, Nov 25, 2014 at 1:55 PM, Rob Mensching <[hidden email]> wrote:

Breaking change to BVariantCopy(). What if we always use the encryption state of Source and always nuke Target’s encryption state (like you are on variant.cpp:271)? Then we just need to get the Source state set correctly before copying.

 

I think that makes a lot of sense anyway, right?  Target encryption state matches Source encryption state on success.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 11:47 AM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Rob Mensching-7

Seems like the only avoidable encrypt/decrypt cycle is when variables are parsed from XML, right? If so, the current code isn’t terribly efficient as it is. Creating an “BVariantSetEveryting()” (better name needed) that provided a value (coerced string?), type and encryption state would be a better way to address that code.

 

Right now, we (FireGiant) are doing the smallest fix possible so hidden variables can be set. We are not doing the bigger better fix to actually implement the encryption for uninitialized hidden variables (without breaking API changes).

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 12:02 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

That works, too.  That just means at least one additional encryption/decryption cycle, which I was trying to avoid.

 

On Tue, Nov 25, 2014 at 1:55 PM, Rob Mensching <[hidden email]> wrote:

Breaking change to BVariantCopy(). What if we always use the encryption state of Source and always nuke Target’s encryption state (like you are on variant.cpp:271)? Then we just need to get the Source state set correctly before copying.

 

I think that makes a lot of sense anyway, right?  Target encryption state matches Source encryption state on success.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 11:47 AM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

SeanHall
No, to make encryption work properly while still calling BVariantCopy without changing its signature, the caller has to call BVariantSetEncryption on the source first (this is the avoidable encryption/decryption cycle).  Which I did in this commit, all non-breaking changes that fixes all of the bug: https://github.com/rseanhall/wix3/commit/4fddecd41bbd33036d773c8190e19919665fa2b5.

FireGiant's pull request fixes the Hidden variable bug, and could fix the whole bug if all callers of BVariantCopy properly set the target's encryption state before calling.  Using the target's encryption seems counter intuitive to me though, especially since it's an out parameter.

On Tue, Nov 25, 2014 at 2:36 PM, Rob Mensching <[hidden email]> wrote:

Seems like the only avoidable encrypt/decrypt cycle is when variables are parsed from XML, right? If so, the current code isn’t terribly efficient as it is. Creating an “BVariantSetEveryting()” (better name needed) that provided a value (coerced string?), type and encryption state would be a better way to address that code.

 

Right now, we (FireGiant) are doing the smallest fix possible so hidden variables can be set. We are not doing the bigger better fix to actually implement the encryption for uninitialized hidden variables (without breaking API changes).

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 12:02 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

That works, too.  That just means at least one additional encryption/decryption cycle, which I was trying to avoid.

 

On Tue, Nov 25, 2014 at 1:55 PM, Rob Mensching <[hidden email]> wrote:

Breaking change to BVariantCopy(). What if we always use the encryption state of Source and always nuke Target’s encryption state (like you are on variant.cpp:271)? Then we just need to get the Source state set correctly before copying.

 

I think that makes a lot of sense anyway, right?  Target encryption state matches Source encryption state on success.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 11:47 AM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs



------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Rob Mensching-7

Ahh, yes, right. The root issue is that the Source encryption state is rarely set “correctly”.   I 100% agree that using the target’s encryption state is weird. I guess what is needed is a “BVariantCopyEx()” (better name’s welcome) that is what you had originally to specify the final desired encryption state (i.e. you were right).

 

I still stand by my comment though that the variables parsing from XML using variant as it does is pretty inefficient. <wink/>

 

 

Note: the FireGiant change was the direct fix that was tested to unblock other processes happening here.

 

 

Note2: The comment “// Encryption here (and decryption later inside BVariantCopy) could have been optimized away with breaking change.” is going look very strange over time. It’s much better for comments to explain why something was done vs. explaining that something that was not done could have been done. The latter will be very confusing (or frustrating) trying to figure out what the actual issue is. <smile/>

 

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 1:19 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

No, to make encryption work properly while still calling BVariantCopy without changing its signature, the caller has to call BVariantSetEncryption on the source first (this is the avoidable encryption/decryption cycle).  Which I did in this commit, all non-breaking changes that fixes all of the bug: https://github.com/rseanhall/wix3/commit/4fddecd41bbd33036d773c8190e19919665fa2b5.

 

FireGiant's pull request fixes the Hidden variable bug, and could fix the whole bug if all callers of BVariantCopy properly set the target's encryption state before calling.  Using the target's encryption seems counter intuitive to me though, especially since it's an out parameter.

 

On Tue, Nov 25, 2014 at 2:36 PM, Rob Mensching <[hidden email]> wrote:

Seems like the only avoidable encrypt/decrypt cycle is when variables are parsed from XML, right? If so, the current code isn’t terribly efficient as it is. Creating an “BVariantSetEveryting()” (better name needed) that provided a value (coerced string?), type and encryption state would be a better way to address that code.

 

Right now, we (FireGiant) are doing the smallest fix possible so hidden variables can be set. We are not doing the bigger better fix to actually implement the encryption for uninitialized hidden variables (without breaking API changes).

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 12:02 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

That works, too.  That just means at least one additional encryption/decryption cycle, which I was trying to avoid.

 

On Tue, Nov 25, 2014 at 1:55 PM, Rob Mensching <[hidden email]> wrote:

Breaking change to BVariantCopy(). What if we always use the encryption state of Source and always nuke Target’s encryption state (like you are on variant.cpp:271)? Then we just need to get the Source state set correctly before copying.

 

I think that makes a lot of sense anyway, right?  Target encryption state matches Source encryption state on success.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 11:47 AM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

SeanHall
My original idea was to create a BVariantSetVariant(__in BURN_VARIANT* pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost an exact copy of BVariantCopy.  Maybe that was the right way to go?  I just wish I was right on the first pull request way back when :)

Hmm, I wonder if I introduced that inefficiency in the XML parsing?  Hopefully not.

Note2: That's why I didn't submit it in a pull request :)  I find it's much easier to talk about code changes when you can actually see it.


On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <[hidden email]> wrote:

Ahh, yes, right. The root issue is that the Source encryption state is rarely set “correctly”.   I 100% agree that using the target’s encryption state is weird. I guess what is needed is a “BVariantCopyEx()” (better name’s welcome) that is what you had originally to specify the final desired encryption state (i.e. you were right).

 

I still stand by my comment though that the variables parsing from XML using variant as it does is pretty inefficient. <wink/>

 

 

Note: the FireGiant change was the direct fix that was tested to unblock other processes happening here.

 

 

Note2: The comment “// Encryption here (and decryption later inside BVariantCopy) could have been optimized away with breaking change.” is going look very strange over time. It’s much better for comments to explain why something was done vs. explaining that something that was not done could have been done. The latter will be very confusing (or frustrating) trying to figure out what the actual issue is. <smile/>

 

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Rob Mensching-7

BVariantSetVariant() isn’t bad. Maybe, BVariantAssign()?  I think the important trick is to control encryption state, right? Almost want a tri-state enum:

 

HRESULT BVariantAssign(

  __in BURN_VARIANT* pTarget,

  __in BURN_VARIANT* pSource,

  __in BURN_VARIANT_ENCRYPTION state // unencrypted, encrypted, follow source

  );

 

…or something like that.

 

 

Note2: Now that you mention it, I like it.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 1:53 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

My original idea was to create a BVariantSetVariant(__in BURN_VARIANT* pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost an exact copy of BVariantCopy.  Maybe that was the right way to go?  I just wish I was right on the first pull request way back when :)

 

Hmm, I wonder if I introduced that inefficiency in the XML parsing?  Hopefully not.

 

Note2: That's why I didn't submit it in a pull request :)  I find it's much easier to talk about code changes when you can actually see it.

 

 

On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <[hidden email]> wrote:

Ahh, yes, right. The root issue is that the Source encryption state is rarely set “correctly”.   I 100% agree that using the target’s encryption state is weird. I guess what is needed is a “BVariantCopyEx()” (better name’s welcome) that is what you had originally to specify the final desired encryption state (i.e. you were right).

 

I still stand by my comment though that the variables parsing from XML using variant as it does is pretty inefficient. <wink/>

 

 

Note: the FireGiant change was the direct fix that was tested to unblock other processes happening here.

 

 

Note2: The comment “// Encryption here (and decryption later inside BVariantCopy) could have been optimized away with breaking change.” is going look very strange over time. It’s much better for comments to explain why something was done vs. explaining that something that was not done could have been done. The latter will be very confusing (or frustrating) trying to figure out what the actual issue is. <smile/>

 

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Bob Arnson-6
Is the existing PR from Team FireGiant still good to go? Or should we wait for a conclusion to this thread?

On 25-Nov-14 19:18, Rob Mensching wrote:

BVariantSetVariant() isn’t bad. Maybe, BVariantAssign()?  I think the important trick is to control encryption state, right? Almost want a tri-state enum:

 

HRESULT BVariantAssign(

  __in BURN_VARIANT* pTarget,

  __in BURN_VARIANT* pSource,

  __in BURN_VARIANT_ENCRYPTION state // unencrypted, encrypted, follow source

  );

 

…or something like that.

 

 

Note2: Now that you mention it, I like it.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [[hidden email]]
Sent: Tuesday, November 25, 2014 1:53 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

My original idea was to create a BVariantSetVariant(__in BURN_VARIANT* pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost an exact copy of BVariantCopy.  Maybe that was the right way to go?  I just wish I was right on the first pull request way back when :)

 

Hmm, I wonder if I introduced that inefficiency in the XML parsing?  Hopefully not.

 

Note2: That's why I didn't submit it in a pull request :)  I find it's much easier to talk about code changes when you can actually see it.

 

 

On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <[hidden email]> wrote:

Ahh, yes, right. The root issue is that the Source encryption state is rarely set “correctly”.   I 100% agree that using the target’s encryption state is weird. I guess what is needed is a “BVariantCopyEx()” (better name’s welcome) that is what you had originally to specify the final desired encryption state (i.e. you were right).

 

I still stand by my comment though that the variables parsing from XML using variant as it does is pretty inefficient. <wink/>

 

 

Note: the FireGiant change was the direct fix that was tested to unblock other processes happening here.

 

 

Note2: The comment “// Encryption here (and decryption later inside BVariantCopy) could have been optimized away with breaking change.” is going look very strange over time. It’s much better for comments to explain why something was done vs. explaining that something that was not done could have been done. The latter will be very confusing (or frustrating) trying to figure out what the actual issue is. <smile/>

 

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/



------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk


_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs

-- 
sig://boB
http://joyofsetup.com/

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Rob Mensching-7

If you want, you could take that just in case you want to push a smaller fix back to WiX v3.9.

 

It does work.

 

A better (and probably bigger) fix (one that addresses the second part of the bug) still is necessary.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Bob Arnson [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 8:05 PM
To: [hidden email]
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

Is the existing PR from Team FireGiant still good to go? Or should we wait for a conclusion to this thread?

On 25-Nov-14 19:18, Rob Mensching wrote:

BVariantSetVariant() isn’t bad. Maybe, BVariantAssign()?  I think the important trick is to control encryption state, right? Almost want a tri-state enum:

 

HRESULT BVariantAssign(

  __in BURN_VARIANT* pTarget,

  __in BURN_VARIANT* pSource,

  __in BURN_VARIANT_ENCRYPTION state // unencrypted, encrypted, follow source

  );

 

…or something like that.

 

 

Note2: Now that you mention it, I like it.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [[hidden email]]
Sent: Tuesday, November 25, 2014 1:53 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

My original idea was to create a BVariantSetVariant(__in BURN_VARIANT* pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost an exact copy of BVariantCopy.  Maybe that was the right way to go?  I just wish I was right on the first pull request way back when :)

 

Hmm, I wonder if I introduced that inefficiency in the XML parsing?  Hopefully not.

 

Note2: That's why I didn't submit it in a pull request :)  I find it's much easier to talk about code changes when you can actually see it.

 

 

On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <[hidden email]> wrote:

Ahh, yes, right. The root issue is that the Source encryption state is rarely set “correctly”.   I 100% agree that using the target’s encryption state is weird. I guess what is needed is a “BVariantCopyEx()” (better name’s welcome) that is what you had originally to specify the final desired encryption state (i.e. you were right).

 

I still stand by my comment though that the variables parsing from XML using variant as it does is pretty inefficient. <wink/>

 

 

Note: the FireGiant change was the direct fix that was tested to unblock other processes happening here.

 

 

Note2: The comment “// Encryption here (and decryption later inside BVariantCopy) could have been optimized away with breaking change.” is going look very strange over time. It’s much better for comments to explain why something was done vs. explaining that something that was not done could have been done. The latter will be very confusing (or frustrating) trying to figure out what the actual issue is. <smile/>

 

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/




------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk




_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs



-- 
sig://boB
http://joyofsetup.com/

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

SeanHall
I submitted my final answer, it should fix the whole bug: https://github.com/wixtoolset/wix3/pull/181

On Tue, Nov 25, 2014 at 10:34 PM, Rob Mensching <[hidden email]> wrote:

If you want, you could take that just in case you want to push a smaller fix back to WiX v3.9.

 

It does work.

 

A better (and probably bigger) fix (one that addresses the second part of the bug) still is necessary.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Bob Arnson [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 8:05 PM
To: [hidden email]
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

Is the existing PR from Team FireGiant still good to go? Or should we wait for a conclusion to this thread?

On 25-Nov-14 19:18, Rob Mensching wrote:

BVariantSetVariant() isn’t bad. Maybe, BVariantAssign()?  I think the important trick is to control encryption state, right? Almost want a tri-state enum:

 

HRESULT BVariantAssign(

  __in BURN_VARIANT* pTarget,

  __in BURN_VARIANT* pSource,

  __in BURN_VARIANT_ENCRYPTION state // unencrypted, encrypted, follow source

  );

 

…or something like that.

 

 

Note2: Now that you mention it, I like it.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [[hidden email]]
Sent: Tuesday, November 25, 2014 1:53 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

My original idea was to create a BVariantSetVariant(__in BURN_VARIANT* pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost an exact copy of BVariantCopy.  Maybe that was the right way to go?  I just wish I was right on the first pull request way back when :)

 

Hmm, I wonder if I introduced that inefficiency in the XML parsing?  Hopefully not.

 

Note2: That's why I didn't submit it in a pull request :)  I find it's much easier to talk about code changes when you can actually see it.

 

 

On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <[hidden email]> wrote:

Ahh, yes, right. The root issue is that the Source encryption state is rarely set “correctly”.   I 100% agree that using the target’s encryption state is weird. I guess what is needed is a “BVariantCopyEx()” (better name’s welcome) that is what you had originally to specify the final desired encryption state (i.e. you were right).

 

I still stand by my comment though that the variables parsing from XML using variant as it does is pretty inefficient. <wink/>

 

 

Note: the FireGiant change was the direct fix that was tested to unblock other processes happening here.

 

 

Note2: The comment “// Encryption here (and decryption later inside BVariantCopy) could have been optimized away with breaking change.” is going look very strange over time. It’s much better for comments to explain why something was done vs. explaining that something that was not done could have been done. The latter will be very confusing (or frustrating) trying to figure out what the actual issue is. <smile/>

 

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/




------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk




_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs



-- 
sig://boB
http://joyofsetup.com/

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs



------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

SeanHall
In reply to this post by Rob Mensching-7
When I look at the current usage of BVariantCopy, I see two different uses.

The first usage is making a local copy of the variant, this usage matches the name and what I had in mind when I was modifying it.  Here we always want the local copy's (pTarget) encryption state to match the original (pSource).

The second usage is being lazy, because what it really wants to do is set the target's value to the source's value, making BVariantCopy figure out which type the source is.  This is why it makes sense to me to name this like the other BVariantSet* methods.  From there it makes sense to me that just like the other BVariantSet* methods, the encryption state shouldn't change.

After typing that out, I now see why you were thinking about BVariantAssign.  VariablesParseFromXml calls BVariantCopy with my second usage.  But it immediately calls BVariantSetEncryption, so it's actually a third kind of usage - setting the target's value and encryption state at the same time.  On the other hand, I have a feeling that after optimizing that code it will not be lazy and will call the appropriate BVariantSet* method directly, with a BVariantSetEncryption call at the end.

This is my long winded answer to Rob's comment on my pull request: the scenario where the source is encrypted but the target is not encrypted can't happen in the current code.  And I don't see a valid use case for it either.  More comments are always good, though :).  I'll add some.

On Tue, Nov 25, 2014 at 6:18 PM, Rob Mensching <[hidden email]> wrote:

BVariantSetVariant() isn’t bad. Maybe, BVariantAssign()?  I think the important trick is to control encryption state, right? Almost want a tri-state enum:

 

HRESULT BVariantAssign(

  __in BURN_VARIANT* pTarget,

  __in BURN_VARIANT* pSource,

  __in BURN_VARIANT_ENCRYPTION state // unencrypted, encrypted, follow source

  );

 

…or something like that.

 

 

Note2: Now that you mention it, I like it.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 1:53 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

My original idea was to create a BVariantSetVariant(__in BURN_VARIANT* pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost an exact copy of BVariantCopy.  Maybe that was the right way to go?  I just wish I was right on the first pull request way back when :)

 

Hmm, I wonder if I introduced that inefficiency in the XML parsing?  Hopefully not.

 

Note2: That's why I didn't submit it in a pull request :)  I find it's much easier to talk about code changes when you can actually see it.

 

 

On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <[hidden email]> wrote:

Ahh, yes, right. The root issue is that the Source encryption state is rarely set “correctly”.   I 100% agree that using the target’s encryption state is weird. I guess what is needed is a “BVariantCopyEx()” (better name’s welcome) that is what you had originally to specify the final desired encryption state (i.e. you were right).

 

I still stand by my comment though that the variables parsing from XML using variant as it does is pretty inefficient. <wink/>

 

 

Note: the FireGiant change was the direct fix that was tested to unblock other processes happening here.

 

 

Note2: The comment “// Encryption here (and decryption later inside BVariantCopy) could have been optimized away with breaking change.” is going look very strange over time. It’s much better for comments to explain why something was done vs. explaining that something that was not done could have been done. The latter will be very confusing (or frustrating) trying to figure out what the actual issue is. <smile/>

 

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs



------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

Rob Mensching-7

A’ight, cool. I’m with you on this. There is only one thing. I’m not big on the name BVariantSetVariant().  I left this comment on the PR:

 

I don't really like the name `BVariantSetVariant`. I get what it is saying but it sounds funny to me. It also doesn't really say that it isn't setting the encryption state (I view the encryption state as part of the *variant*). What about `BVariantSetValue`? That scopes it more to just the value part of the variant... thoughts?

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Thursday, November 27, 2014 7:59 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

When I look at the current usage of BVariantCopy, I see two different uses.

 

The first usage is making a local copy of the variant, this usage matches the name and what I had in mind when I was modifying it.  Here we always want the local copy's (pTarget) encryption state to match the original (pSource).

 

The second usage is being lazy, because what it really wants to do is set the target's value to the source's value, making BVariantCopy figure out which type the source is.  This is why it makes sense to me to name this like the other BVariantSet* methods.  From there it makes sense to me that just like the other BVariantSet* methods, the encryption state shouldn't change.

 

After typing that out, I now see why you were thinking about BVariantAssign.  VariablesParseFromXml calls BVariantCopy with my second usage.  But it immediately calls BVariantSetEncryption, so it's actually a third kind of usage - setting the target's value and encryption state at the same time.  On the other hand, I have a feeling that after optimizing that code it will not be lazy and will call the appropriate BVariantSet* method directly, with a BVariantSetEncryption call at the end.

 

This is my long winded answer to Rob's comment on my pull request: the scenario where the source is encrypted but the target is not encrypted can't happen in the current code.  And I don't see a valid use case for it either.  More comments are always good, though :).  I'll add some.

 

On Tue, Nov 25, 2014 at 6:18 PM, Rob Mensching <[hidden email]> wrote:

BVariantSetVariant() isn’t bad. Maybe, BVariantAssign()?  I think the important trick is to control encryption state, right? Almost want a tri-state enum:

 

HRESULT BVariantAssign(

  __in BURN_VARIANT* pTarget,

  __in BURN_VARIANT* pSource,

  __in BURN_VARIANT_ENCRYPTION state // unencrypted, encrypted, follow source

  );

 

…or something like that.

 

 

Note2: Now that you mention it, I like it.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 1:53 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

My original idea was to create a BVariantSetVariant(__in BURN_VARIANT* pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost an exact copy of BVariantCopy.  Maybe that was the right way to go?  I just wish I was right on the first pull request way back when :)

 

Hmm, I wonder if I introduced that inefficiency in the XML parsing?  Hopefully not.

 

Note2: That's why I didn't submit it in a pull request :)  I find it's much easier to talk about code changes when you can actually see it.

 

 

On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <[hidden email]> wrote:

Ahh, yes, right. The root issue is that the Source encryption state is rarely set “correctly”.   I 100% agree that using the target’s encryption state is weird. I guess what is needed is a “BVariantCopyEx()” (better name’s welcome) that is what you had originally to specify the final desired encryption state (i.e. you were right).

 

I still stand by my comment though that the variables parsing from XML using variant as it does is pretty inefficient. <wink/>

 

 

Note: the FireGiant change was the direct fix that was tested to unblock other processes happening here.

 

 

Note2: The comment “// Encryption here (and decryption later inside BVariantCopy) could have been optimized away with breaking change.” is going look very strange over time. It’s much better for comments to explain why something was done vs. explaining that something that was not done could have been done. The latter will be very confusing (or frustrating) trying to figure out what the actual issue is. <smile/>

 

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs
Reply | Threaded
Open this post in threaded view
|

Re: Just opened bug 4609 against v3.9

SeanHall
I like it, I'll update it to BVariantSetValue.

On Thu, Dec 4, 2014 at 4:08 PM, Rob Mensching <[hidden email]> wrote:

A’ight, cool. I’m with you on this. There is only one thing. I’m not big on the name BVariantSetVariant().  I left this comment on the PR:

 

I don't really like the name `BVariantSetVariant`. I get what it is saying but it sounds funny to me. It also doesn't really say that it isn't setting the encryption state (I view the encryption state as part of the *variant*). What about `BVariantSetValue`? That scopes it more to just the value part of the variant... thoughts?

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Thursday, November 27, 2014 7:59 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

When I look at the current usage of BVariantCopy, I see two different uses.

 

The first usage is making a local copy of the variant, this usage matches the name and what I had in mind when I was modifying it.  Here we always want the local copy's (pTarget) encryption state to match the original (pSource).

 

The second usage is being lazy, because what it really wants to do is set the target's value to the source's value, making BVariantCopy figure out which type the source is.  This is why it makes sense to me to name this like the other BVariantSet* methods.  From there it makes sense to me that just like the other BVariantSet* methods, the encryption state shouldn't change.

 

After typing that out, I now see why you were thinking about BVariantAssign.  VariablesParseFromXml calls BVariantCopy with my second usage.  But it immediately calls BVariantSetEncryption, so it's actually a third kind of usage - setting the target's value and encryption state at the same time.  On the other hand, I have a feeling that after optimizing that code it will not be lazy and will call the appropriate BVariantSet* method directly, with a BVariantSetEncryption call at the end.

 

This is my long winded answer to Rob's comment on my pull request: the scenario where the source is encrypted but the target is not encrypted can't happen in the current code.  And I don't see a valid use case for it either.  More comments are always good, though :).  I'll add some.

 

On Tue, Nov 25, 2014 at 6:18 PM, Rob Mensching <[hidden email]> wrote:

BVariantSetVariant() isn’t bad. Maybe, BVariantAssign()?  I think the important trick is to control encryption state, right? Almost want a tri-state enum:

 

HRESULT BVariantAssign(

  __in BURN_VARIANT* pTarget,

  __in BURN_VARIANT* pSource,

  __in BURN_VARIANT_ENCRYPTION state // unencrypted, encrypted, follow source

  );

 

…or something like that.

 

 

Note2: Now that you mention it, I like it.

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/

 

From: Sean Hall [mailto:[hidden email]]
Sent: Tuesday, November 25, 2014 1:53 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9

 

My original idea was to create a BVariantSetVariant(__in BURN_VARIANT* pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost an exact copy of BVariantCopy.  Maybe that was the right way to go?  I just wish I was right on the first pull request way back when :)

 

Hmm, I wonder if I introduced that inefficiency in the XML parsing?  Hopefully not.

 

Note2: That's why I didn't submit it in a pull request :)  I find it's much easier to talk about code changes when you can actually see it.

 

 

On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <[hidden email]> wrote:

Ahh, yes, right. The root issue is that the Source encryption state is rarely set “correctly”.   I 100% agree that using the target’s encryption state is weird. I guess what is needed is a “BVariantCopyEx()” (better name’s welcome) that is what you had originally to specify the final desired encryption state (i.e. you were right).

 

I still stand by my comment though that the variables parsing from XML using variant as it does is pretty inefficient. <wink/>

 

 

Note: the FireGiant change was the direct fix that was tested to unblock other processes happening here.

 

 

Note2: The comment “// Encryption here (and decryption later inside BVariantCopy) could have been optimized away with breaking change.” is going look very strange over time. It’s much better for comments to explain why something was done vs. explaining that something that was not done could have been done. The latter will be very confusing (or frustrating) trying to figure out what the actual issue is. <smile/>

 

 

_______________________________________________________________

FireGiant  |  Dedicated support for the WiX toolset  |  http://www.firegiant.com/


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs

 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs



------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/wix-devs