Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zxp-provider can return a cmd/a path to ZXPSignCmd tool. #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Lilipi
Copy link

@Lilipi Lilipi commented Jul 23, 2019

  • Cut and pass correctly command and arguments to spawn command
  • Otherwise works like before

Links with #36 and codearoni/zxp-provider#14

@Lilipi Lilipi force-pushed the feature/zxpProviderVariable branch from fb6d711 to a35dcb7 Compare July 25, 2019 08:38
@Lilipi Lilipi changed the title Zxp-provider can return a cmd/a path to ZXPSignCmd tool. [WIP] Zxp-provider can return a cmd/a path to ZXPSignCmd tool. Jul 25, 2019
@fcamarlinghi
Copy link
Owner

Thanks for the PR, but I'm not a fan of the proposed changes.

The main concern is that the resulting code is far more complex and difficult to read than the original implementation. While I understand you need to split the command returned by zxp-provider in order to pass the ZXPSignCmd path to Wine, it's a very particular use case and as such it should be kept outside grunt-cep or implemented in a different way. A possible solution might be to refactor the code to build the full command string manually and run it by setting shell: true in the options passed to grunt.util.spawn. However, it's a pretty big change which might break existing setups and I unfortunately don't have enough time in my hands right now to look into that.

The second concern is that you implemented the new functionality only for the "sign" command. I understand that's what you need in your particular use case, but it creates inconsistencies in the API when using the "certificate" command. A better place to implement support for the new require('zxp-provider').env option would be outside of the single commands, where the "zxp_path" is first loaded.

I think your best bet in the immediate future is to use a custom version of grunt-cep in your setup.

@Lilipi
Copy link
Author

Lilipi commented Jul 25, 2019

Thanks for your feedback ,
I've updated my PR on zxp-provider this morning and didn't have the time to go back to this one. I did this one really fast so clearly it isn't coded in the best way.

When my PR on zxp-provider will be accepted (if it happens) ; I will update this one too to load zxp path correctly at the first time.

Your feedbacks are true, I didn't pay attention to the other commands and will correct that later too.

I'm using my "custom" grunt-cep version on my setup right now so it's not a problem, but it would be great if it could be included the right way in your module.

I will work on it if you agree.

Edit : PR updated

…zxp-provider PR to be accepted).

/!\ needs zxp-provider upgrade

- use zxp-provider.env new method before calling its ".bin" method
- build whole command line with passing arguments
- use child_process.spawn with "bash: true" to execute command

Tested on :
- Windows without environment variable (as worked before)
- Windows with environment variable
- Linux with environment variable
@Lilipi Lilipi force-pushed the feature/zxpProviderVariable branch from a35dcb7 to 4e89a62 Compare July 26, 2019 14:11
@Lilipi Lilipi changed the title [WIP] Zxp-provider can return a cmd/a path to ZXPSignCmd tool. Zxp-provider can return a cmd/a path to ZXPSignCmd tool. Jul 26, 2019
@fcamarlinghi
Copy link
Owner

Sorry for the delay in getting back to you and thanks for updating the PR. I'll test and integrate the changes as soon as I find some time.

@Lilipi
Copy link
Author

Lilipi commented Aug 6, 2019

Hi,
Don't worry, no problem.
My PR on zxp-provider module is not accepted yet.
Thanks

@Lilipi
Copy link
Author

Lilipi commented Sep 24, 2019

Hi,
It seems like zxp-provider's owner doesn't want to update his module.
I will modify this PR to be indepedent of this upgrade (and only modify how command is executed)

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants