Skip to content

Also install future in Windows startup script.#492

Merged
oliverchang merged 1 commit into
masterfrom
windows-pip
May 27, 2019
Merged

Also install future in Windows startup script.#492
oliverchang merged 1 commit into
masterfrom
windows-pip

Conversation

@oliverchang

Copy link
Copy Markdown
Collaborator

No description provided.

@googlebot googlebot added the cla: yes CLA signed. label May 27, 2019
@oliverchang

Copy link
Copy Markdown
Collaborator Author

Also, what's the proper way to install this on our macOS machines these days?

@inferno-chromium

inferno-chromium commented May 27, 2019

Copy link
Copy Markdown
Collaborator

actually why do we need in startup script ? we have this src/requirements.txt for bots and we set pythonpath very early at start. do we have other scripts like docker ones that need future (e.g. 4bf8622)

@oliverchang

oliverchang commented May 27, 2019

Copy link
Copy Markdown
Collaborator Author

This is just to make it safer to run the remainder of the fixers, which will be adding a lot of these future imports. For instance, I'm not even sure how https://github.com/google/clusterfuzz/blob/master/src/python/bot/startup/run_bot.py#L20 works right now, because it happens before the modules.fix_module_search_paths(), which is what adds third_party to sys.path.

Having this installed on the CI but not the base bot images is also a mismatch that would hide issues.

@inferno-chromium

inferno-chromium commented May 27, 2019

Copy link
Copy Markdown
Collaborator

ah right, i think all these builtins import should be moved after fix_module_search_paths for safety and to work across mac, android bots. and might have to be careful on run.py and run_heartbeat too. good to have in base image and windows, so lgtm.

@oliverchang oliverchang merged commit facf65e into master May 27, 2019
@inferno-chromium inferno-chromium deleted the windows-pip branch May 28, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes CLA signed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants