Skip to content

fix: change Makefile task name #216

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

Merged
merged 7 commits into from
Apr 16, 2025
Merged

fix: change Makefile task name #216

merged 7 commits into from
Apr 16, 2025

Conversation

erral
Copy link
Member

@erral erral commented Apr 16, 2025

Fixes #215

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Changing the name of the target means it will run unless there is a file named venv, which is not the best solution.

I think the real problem is that the requirements-mxdev.txt target doesn't list its dependencies, see my explanation here: #215 (comment)

Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

This is definitely not the correct fix. See my comment in #215 (comment) and a related issue in documentation with a suggested and untested fix plone/documentation#1938.

@ksuess
Copy link
Member

ksuess commented Apr 16, 2025

I would like to see the prerequisites also in templates/add-ons/backend/{{ cookiecutter.__folder_name }}/Makefile, cause I already had the case that I wanted to test the to be developed backend-add on with another add-on (not part of the repo), just to see that they play well together.

@erral
Copy link
Member Author

erral commented Apr 16, 2025

@ksuess I have updated the PR with those changes

Copy link
Member

@ksuess ksuess left a comment

Choose a reason for hiding this comment

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

Thanks for the adjustments!

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

This looks right (or at least, more right 😛) to me

@davisagli davisagli merged commit 563c324 into main Apr 16, 2025
9 checks passed
@davisagli davisagli deleted the erral-venv-folder branch April 16, 2025 22:38
@davisagli
Copy link
Member

@erral Thanks for the fix!

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.

Can't install backend addons after first run of make install
4 participants