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

ensure venv for Python tools #162

Merged
merged 6 commits into from
Jun 6, 2023
Merged

Conversation

bertsky
Copy link
Contributor

@bertsky bertsky commented May 5, 2023

No description provided.

@bertsky bertsky mentioned this pull request May 5, 2023
@bertsky
Copy link
Contributor Author

bertsky commented Jun 6, 2023

It's been one month – can we please get ahead with this? (We still need to update ocrd_fileformat...)

@stweil stweil merged commit 3e32ef6 into UB-Mannheim:master Jun 6, 2023
Comment on lines +62 to +67
# create+activate a Python venv if not already active
if [ -z "$(VIRTUAL_ENV)" ]; then \
$(PYTHON) -m venv $(SHAREDIR)/venv && \
. $(SHAREDIR)/venv/bin/activate && \
pip install -U pip; \
fi && $(MAKE) -C vendor all
Copy link
Member

@stweil stweil Feb 13, 2024

Choose a reason for hiding this comment

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

@bertsky, @kba, these lines break make all in a default build (VIRTUAL_ENV not set, SHAREDIR=/usr/local/share).

Making a virtual Python environment will fail because of missing write permission.

Using sudo will make things even worse because of git conflicts with different users, and it will also create files owned by root in the user's file tree.

Is it okay if SHAREDIR is writable and already contains a venv to overwrite it by a new create command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO $(or $(VIRTUAL_ENV), $(SHAREDIR)/venv) should be an explicit prerequisite of vendor (to rule out overwriting and save time).

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.

3 participants