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

[flashing] Support the check for product on Windows. Fixes MER#1961 #29

Closed
wants to merge 1 commit into from

Conversation

Thaodan
Copy link
Contributor

@Thaodan Thaodan commented Sep 23, 2018

Modifers @Devices@ to support matching on supported devices and
changes the layout of the comment containing all supported devices as in flash.sh

@Thaodan Thaodan changed the title [flashing] Support the check for product on windows. Fixes MER#1961 [flashing] Support the check for product on Windows. Fixes MER#1961 Sep 23, 2018
@Thaodan Thaodan force-pushed the batch_checkproduct branch 5 times, most recently from a698604 to 2ddc528 Compare September 26, 2018 14:08
@mkosola
Copy link
Contributor

mkosola commented Oct 12, 2018

At least this does not work for me... I get following error:
Searching a compatible device... GOTO was unexpected at this time.

Also product is now in two places on windows flashing script

@Thaodan Thaodan force-pushed the batch_checkproduct branch 3 times, most recently from b2e063f to 8851c79 Compare October 22, 2018 17:04
@Thaodan
Copy link
Contributor Author

Thaodan commented Oct 22, 2018

I updated the pr to the current changes but need to add a second string to match devices found.
However I still need to test this later.

@@ -142,21 +142,27 @@ FILES_TO_COPY="*.urls"
if [ "@DEVICEMODEL@" = "h3113" ]; then
sed -e "s/\@DEVICES\@/\-e \"H3113\" -e \"H3123\" -e \"H3133\"/g" -i flash.sh
sed -e "s/\@DEVICES\@/:: H3113\r\n:: H3123\r\n:: H3133/g" -i flash-on-windows.bat
sed -e "s/\@DEVICES_FINDSTR\@/H3123 H3133/g" -i flash-on-windows.bat
Copy link
Contributor

@mlehtima mlehtima Jan 21, 2020

Choose a reason for hiding this comment

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

Is there any reason to add a second sed command for windows script? You could just fix the existing one as that is not really used for anything just to add a comment to the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I've changed the layout of the which contained @Devices@ to do this.

@@ -58,33 +58,40 @@ exit /b 1

:no_error_serialnumbers

for %%d in ( %serialnumbers% ) do (
set fastbootcmd=%fastbootcmd_no_devic% -s %%d
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, should have fastbootcmd_no_device

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@mlehtima
Copy link
Contributor

mlehtima commented Jan 21, 2020

Wondering if we need to support having multiple devices connected? That just makes the script somewhat more complicated and it's quite rare use case to have multiple devices of different type in fastboot mode connected to the computer.

If we would only support one connected device then the changes in this PR would be much more simple, only a small loop to check for matching device after checking that there is only one device.

Modifers @Devices@ to support matching on supported devices and
changes the layout of the comment containing all supported devices as in flash.sh
Copy link
Contributor Author

@Thaodan Thaodan left a comment

Choose a reason for hiding this comment

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

The issue is that this is as batch script which is painfully to maintain.
I think rewriting this script in powershell would be the best solution in the long run.

@jovirkku
Copy link
Contributor

jovirkku commented Feb 3, 2020

At least for now, our requirement has been to also support Windows 7 and Windows 8. Windows 7 does not have PowerShell (not sure about Win 8). Furthermore, there has been some cases where a Windows PC has contained some PowerShell environment variables (defined on that PC) which have caused conflicts with the batch script. So we shouldn't rush into PowerShell before considering the consequences.

@mkosola
Copy link
Contributor

mkosola commented Feb 18, 2020

We had to modify your code a bit and we did new pull request. Fix is here #114

@mkosola mkosola closed this Feb 18, 2020
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.

None yet

4 participants