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

make all SKIP_MODULES=1 to not update modules #137

Merged
merged 2 commits into from
Aug 15, 2020
Merged

make all SKIP_MODULES=1 to not update modules #137

merged 2 commits into from
Aug 15, 2020

Conversation

kba
Copy link
Member

@kba kba commented Aug 4, 2020

No description provided.

Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

Yes, it would be useful for development (e.g. running make after changing submodules but before committing) to have such an option.

But IMHO SKIP_MODULES is not a good name: It sounds like DISABLED_MODULES. How about NO_UPDATES=1?

@kba
Copy link
Member Author

kba commented Aug 15, 2020

Yes, it would be useful for development (e.g. running make after changing submodules but before committing) to have such an option.

But IMHO SKIP_MODULES is not a good name: It sounds like DISABLED_MODULES. How about NO_UPDATES=1?

I thought SKIP_MODULES because it "skips" the make modules target. But NO_UPDATES is fine with me.

@kba kba requested a review from bertsky August 15, 2020 10:42
@stweil stweil merged commit adb5209 into master Aug 15, 2020
@stweil stweil deleted the skip-modules branch August 15, 2020 16:14
@@ -108,11 +111,13 @@ modules: $(OCRD_MODULES)
# but bypass updates if we have no repo here (e.g. Docker build)
ifneq (,$(wildcard .git))
$(OCRD_MODULES): always-update
ifneq ($(NO_UPDATE),1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ifeq ($NO_UPDATE),0) would also work with NO_UPDATE=yes or even NO_UPDATE=anything_not_0. Do you also plan to add information to README.md?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you also plan to add information to README.md?

I wouldn't recommend advertising it too much. After all, it's a developer option...

Copy link
Member Author

Choose a reason for hiding this comment

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

ifeq ($NO_UPDATE),0) would also work with NO_UPDATE=yes or even NO_UPDATE=anything_not_0. Do you also plan to add information to README.md?

You're right, that would make it easier to use but as @bertsky says, this is not something we want users to "try if anything else fails", so I think it's best not to document in the README.md (perhaps in the contributor guide?).

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