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

Remove quotes from the environment path #586

Merged
merged 2 commits into from
May 17, 2021
Merged

Remove quotes from the environment path #586

merged 2 commits into from
May 17, 2021

Conversation

EndrII
Copy link
Member

@EndrII EndrII commented May 3, 2021

fix #581

Note

need to manually tests.

@EndrII EndrII added Bug Something isn't working Windows Windows version labels May 3, 2021
@EndrII EndrII added this to the v1.5 milestone May 3, 2021
@EndrII EndrII requested a review from pzhlkj6612 May 3, 2021 14:09
@EndrII EndrII self-assigned this May 3, 2021
@EndrII EndrII changed the title remove quotes from the envirement path Remove quotes from the environment path May 3, 2021
@pzhlkj6612
Copy link
Collaborator

Hi.

  1. As I said in Environment variable configured by CQtDeployer's own installer on Windows should not be quoted #581 (comment) , unconditionally adding quotes may break the program. I‘m thinking of another way now.
  2. You forgot CQtDeployer/test.pri .
  3. I didn't even know that these environment variables are stored in this repository (that is, QIFData/packages/cqtdeployer*/meta/installscript.js ). I was too careless.

@EndrII
Copy link
Member Author

EndrII commented May 3, 2021

1. As I said in [#581 (comment)](https://github.com/QuasarApp/CQtDeployer/issues/581#issuecomment-829740659) , unconditionally adding quotes may break the program. I‘m thinking of another way now.

now we can check it. look at the test results.

@EndrII
Copy link
Member Author

EndrII commented May 3, 2021

@pzhlkj6612 it seems the extra quotes didn't break anything.
tests of cqtdeployer will be runned and finished successfully.

@EndrII
Copy link
Member Author

EndrII commented May 4, 2021

@pzhlkj6612 please check this fix manually on cmd and powershell
download link

@pzhlkj6612
Copy link
Collaborator

... it seems the extra quotes didn't break anything.

Yes, it does.

Please wait for my further testing.

@pzhlkj6612
Copy link
Collaborator

... it seems the extra quotes didn't break anything.

Yes, it does.

Well, I mean, it didn't break anything in my previous testing.


I need to learn more English.

@EndrII
Copy link
Member Author

EndrII commented May 7, 2021

... it seems the extra quotes didn't break anything.

Yes, it does.

Well, I mean, it didn't break anything in my previous testing.

I need to learn more English.

Do you test on this build?

@EndrII EndrII enabled auto-merge May 7, 2021 07:06
@pzhlkj6612
Copy link
Collaborator

Hi. Since testing CQtDeployer after building needs an already installed CQtDeployer, I want to know what version of CQtDeployer is installed on your CI.

@EndrII
Copy link
Member Author

EndrII commented May 16, 2021

Hi. Since testing CQtDeployer after building needs an already installed CQtDeployer, I want to know what version of CQtDeployer is installed on your CI.

I tested it on CI all is fine.

@pzhlkj6612
Copy link
Collaborator

I mean, the already installed CQtDeployer seems to affect the behavior of test.
Could you please provide the version number of CQtDeployer which is installed on your CI?

@EndrII EndrII disabled auto-merge May 17, 2021 07:47
@EndrII
Copy link
Member Author

EndrII commented May 17, 2021

Tested on build bot CI. (replace stable version of cqtdeployer)

@EndrII EndrII merged commit e522d1a into v1.5 May 17, 2021
@EndrII EndrII deleted the task_581 branch June 16, 2021 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Windows Windows version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants