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

Fix Makefule rules "setup-virtualenv" and "check-program" within invocation of "make setup" #19

Merged
merged 4 commits into from Sep 5, 2019

Conversation

poesel
Copy link
Collaborator

@poesel poesel commented Sep 4, 2019

directories .venv2 & .venv3 do not exist yet when they are tested
since we install python & virtualenv during the build of the image its save to assume they exist

sorry for having the other changes in here, too. I'm still new to git :)

I also don't know why the diff is so big. The actual difference is just two lines.

@amotl
Copy link
Member

amotl commented Sep 4, 2019

Thanks for providing this fix to the sandbox infrastructure. However, we are not able to spot the specific change yet ;].

I don't know why the diff is so big. The actual difference is just two lines.

This might be related to newline issues. You might want to check your editor settings or investigate the diff more thoroughly to find out what might have gone wrong.

Sorry for having the other changes in here, too. I'm still new to git :)

No worries. We will try to cherry-pick the change you described at [1]:

In tools/core.mk muss es unter ‘check-program’: … != “”… heißen, sonst funktioniert der Test nicht.

and after that, this PR should be moot, right?

[1] https://community.hiveeyes.org/t/setup-der-terkin-datenlogger-sandbox-schlagt-fehl/2501/13

@poesel
Copy link
Collaborator Author

poesel commented Sep 5, 2019

Nope. Theres also this:

setup-virtualenv2: check-virtualenv
	@test -e $(python2) && virtualenv --python=python2 --no-site-packages $(venv2path)

setup-virtualenv3: check-virtualenv
	@test -e $(python3) && virtualenv --python=python3 --no-site-packages $(venv3path)
	$(pip3) --quiet install --requirement requirements-dev.txt

because

$(eval venv2path    := .venv2)
and
$(eval python2      := $(venv2path)/bin/python)
this:
@test -e $(python2)

must fail because .venv2 doesn't exist, yet. It is created with:

virtualenv --python=python2 --no-site-packages $(venv2path)

after the test. I've deleted the test because its IMHO superfluous. If python2 wouldn't exist virtualenv would fail anyway.

Same reason for python3 of course.

@amotl
Copy link
Member

amotl commented Sep 5, 2019

Dear @poesel,

thanks for clarifying that. We have been aware that we got these checks wrong and will be happy to merge an appropriate pull request fixing that. However, we still haven't been able to dedicate ourselves to this topic. As we are currently traveling, we would only be able to merge pull requests which don't require any specific attention.

So, we are humbly asking if you could amend this pull request which yields a more distinctive diff between the current master and these fixes. You will be able to push to the same branch and you might want to use git push --force to override eventual rejections there.

Otherwise, please let us know if we should take that job. Then, we will try to catch some time on this.

With kind regards,
Andreas.

@poesel
Copy link
Collaborator Author

poesel commented Sep 5, 2019

Now its smaller. Don't know why it did what it did.

@amotl
Copy link
Member

amotl commented Sep 5, 2019

Thanks for compressing the diff. I will merge these patches as well but would like to mention that the update to the rule check-program does not convince me yet. Invoking this on a macOS machine does not yield any errors at all. As a reference, these are the results when invoked before the update:

# Works
make check-program program=yes

# Croaks
make check-program program=hotzenplotz
ERROR: "hotzenplotz" program not installed.
HINT:
make: *** [check-program] Error 1

@amotl amotl merged commit 3798e8b into hiveeyes:master Sep 5, 2019
@amotl amotl changed the title Fix make setup Fix make setup re. "setup-virtualenv" and "check-program" Sep 5, 2019
@amotl amotl changed the title Fix make setup re. "setup-virtualenv" and "check-program" Fix Makefule rules "setup-virtualenv" and "check-program" within invocation of "make setup" Sep 5, 2019
@poesel poesel deleted the fix-make-setup branch December 24, 2019 09:42
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

2 participants