Skip to content

[RSDK-11469] Add Windows support to the CI#17

Merged
penguinland merged 19 commits intomainfrom
windows_ci
Aug 4, 2025
Merged

[RSDK-11469] Add Windows support to the CI#17
penguinland merged 19 commits intomainfrom
windows_ci

Conversation

@penguinland
Copy link
Contributor

@penguinland penguinland commented Jul 31, 2025

and other refactoring:

  • When you run make setup, the script that runs is now called setup.sh, and it does setup work.
  • There's a new make dist/main target that builds the executable.
  • There's a new step in publish.yml to build and upload on Windows. I've marked the file as one to run on workflow_dispatch so you can manually run it without creating a release, and have gotten that to work.
  • The requirements.txt did not pin versions of stuff, and it rotted and could not find mutually compatible versions of everything. So, I copied the versions pinned in https://github.com/viam-modules/keras-detector/. I was going to pin all versions so this wouldn't happen again, and got surprised that pip freeze gives no output! This happens on both Windows and Linux. So, I gave up on that for the moment.
  • Tried out: everything builds correctly, though I purposely stopped just shy of uploading to the registry.

@penguinland penguinland requested a review from kharijarrett July 31, 2025 16:53
@penguinland
Copy link
Contributor Author

Copy link
Collaborator

@kharijarrett kharijarrett left a comment

Choose a reason for hiding this comment

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

I have some questions tbh. Also, I believe in the build but could you just test it on one of our Windows test machines?

.setup: setup.sh
./setup.sh

setup: .setup
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is confusing. Why are we doing this? Why can't make setup just call the ./setup.sh ? Lmk if I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This question and the next one have the same answer. When we run setup.sh, the last thing it does if everything else succeeds is create a .setup file in the repo. If that file exists, you don't need to re-run setup.sh every time, because it has already been run in the past. The file starts with a period so that it's normally hidden and you don't have to think about it.

So, line 3 is about how to create the .setup file, and line 6 is syntactic sugar so you can run make setup like you'd expect.

We did the same thing in the duplicate image classifier and blurry image classifier, and IIRC this is how first_run scripts in meta.json work.

I should mark setup as PHONY, so it's clearer that .setup is a real file that gets created but setup is not.

source $VIRTUAL_ENV/Scripts/activate # Windows
fi
pip install --prefer-binary -r requirements.txt -U

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, why are we not doing the Pyinstaller stuff here instead of the Makefile? Like you have to activate the environment again and copy it over twice, etc etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is about setup: making sure your machine has everything installed, and only needs to happen once. PyInstaller is about compiling the executable so it can be run, and gets run every time you want to rebuild stuff. It's the same reason that the cloud build section of meta.json has separate "setup" and "build" commands in the cloud build section.

You're right that make setup was previously not only the setup but also built the whole executable. By splitting the two steps out, we can save time not redoing the parts that haven't changed.

Makefile Outdated
ACTIVATE = source .venv/bin/activate
endif
dist/main: .setup src/*
$(ACTIVATE) && python -m PyInstaller --onefile --hidden-import="googleapiclient" --add-data="./src:src" src/main.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Khari and I both disliked the duplicated code in here. I think I've made it somewhat better!

  • Every line in a recipe is run in its own shell, so we can't have 1 line to activate the venv and another line for Pyinstaller
  • The make parser didn't like it when I tried interleaving the if statement with lines ending with && \ (meaning "and if this line succeeds then as a second part of the current statement run the thing on the next line down"): it would complain about being unable to find the endif statement.
  • You can't obviously create variables inside a recipe, but can create them globally outside any recipe. So, create a way to activate the venv globally, and then within the recipe use it and also run PyInstaller.

@penguinland
Copy link
Contributor Author

could you just test it on one of our Windows test machines?

This was a really good call, thanks! There were multiple bugs around actually uploading the .tar.gz file.

The Linux/Mac build still fails its action, but succeeds in uploading entries to the registry. I think I've seen Abe complain that there's a bug in the build action where it mistakenly thinks something has failed even though it succeeded. I'll try to find a Jira ticket for that and ping it again.

Tried on kj-test-windows: seems to work fine, though downloading the new version is slow because it's 400 MB. So, we shouldn't update the uses in production until we're confident there won't be more changes in an upcoming PR.

Copy link
Collaborator

@kharijarrett kharijarrett left a comment

Choose a reason for hiding this comment

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

Yes! This LGTM!

@penguinland penguinland merged commit a74a94d into main Aug 4, 2025
3 of 4 checks passed
@penguinland penguinland deleted the windows_ci branch August 4, 2025 19:40
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