Skip to content

Makefile - Make it a bit more usable #701

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 5 commits into from
Oct 30, 2022

Conversation

omesser
Copy link
Contributor

@omesser omesser commented Oct 23, 2022

  • Adding target to build leo and terraform-provider-iterative binaries in repo root
  • build target now uses the above instead of the current "check-build" (checks validity, but produces no binaries, which might be useful)
  • install_tpi target - built and installed leo bin into tf plugins (probably unwanted behavior) - fixed
  • Added phony directives for phony targets (which also allows for the convenience of make targets which acts as --help)
  • Some light refactoring
  • Addition to gitignore for JB ideas

@omesser omesser requested a review from a team October 23, 2022 22:24
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

Why does the leo binary have a GO{OS,ARCH} suffix? 🤔 I'd assume that local developers won't do that much cross-compilation.

leo-*

# JetBrains IDEs
.idea/*
Copy link
Contributor

@casperdcl casperdcl Oct 27, 2022

Choose a reason for hiding this comment

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

Really dislike adding personal things to .gitignore. People's personal setups should use .git/info/exclude instead

Copy link
Contributor Author

@omesser omesser Oct 27, 2022

Choose a reason for hiding this comment

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

I disagree here I'm afraid 🙏 😄 - it's an assumption that is very problematic. I also suggest to add common IDE .gitignores to any other repo if missing actually, there aren't many common IDE stuff to ignore to cover 95% of cases (super niche editors and custom metadata set-ups aren't relevant).

I think this an opinion vs pragmatism scenario;

Even if it would've been nice to have people systematically use personal configs in theory (good mechanism, that's what its for, might be nice to separate setup-based-ignores and project-based-ignores), in practice, most developers just don't bother with this.
It's enough that those exist in non-negligable numbers to justify removing this as a barrier imo.

Especially a must for open source repos IMO - We can't assume those things on external contributors even if we wanted to assume+enforce using personal git profiles inside a company.

For those who do use personal gitignores - this is harmless, for those who don't - omitting this for cleanliness/aesthetics reason will add needless friction which is real.

To illustrate/support:

Copy link
Contributor Author

@omesser omesser Oct 27, 2022

Choose a reason for hiding this comment

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

Another reason just occurred to me (re: cloud IDE) - the case for ephemeral dev boxes which may not be pre-configured in this way

Copy link
Contributor

Choose a reason for hiding this comment

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

I think ide-specific exclude paths are perfectly fine in .gitignore. It's a low impact change that we're spending an unreasonable amount of time discussing.

@omesser
Copy link
Contributor Author

omesser commented Oct 27, 2022

Why does the leo binary have a GO{OS,ARCH} suffix? 🤔 I'd assume that local developers won't do that much cross-compilation.

@0x2b3bfa0
Just a good practice IMO to make those explicit - always set up things with cross-compilation in mind, at some point someone will test different GOOS/GOARCH and binaries will override and it will be annoying, and no real downsides (it's a suffix, tab-completion removes terminal-unweildliness).

So - just personal preference / practice. If you (or anyone) find this detrimental I'll just remove 😄

@0x2b3bfa0
Copy link
Member

[...] at some point someone will test different GOOS/GOARCH [..] If you (or anyone) find this detrimental I'll just remove

This is too subjective to unbury my “detrimental sledgehammer” but I can't imagine any plausible case where cross-compiling this project during local development would become a commonplace practice. 🤷🏼‍♂️

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

Missing a better suggestion, 💅🏼

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🎉

Copy link
Contributor

@tasdomas tasdomas left a comment

Choose a reason for hiding this comment

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

Let's just land this and move on. A simple Makefile change is stuck in the review phase for 5 days - this should have landed 4.5 days ago

GOARCH=${shell go env GOARCH}
TF_PLUGIN_INSTALL_PATH=~/.terraform.d/plugins/${HOSTNAME}/${NAMESPACE}/${NAME}/${VERSION}/${GOOS}_${GOARCH}
TPI_PATH ?= $(shell pwd)
LEO_PATH ?= $(shell pwd)/cmd/leo
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're only using these paths to specify the build entry point, they can be relative to the Makefile. Actually, since we're only using them in one single location, there's not much point in defining constants for them

leo-*

# JetBrains IDEs
.idea/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ide-specific exclude paths are perfectly fine in .gitignore. It's a low impact change that we're spending an unreasonable amount of time discussing.

@dacbd dacbd enabled auto-merge (squash) October 30, 2022 00:52
@dacbd dacbd merged commit 2d4dc35 into iterative:master Oct 30, 2022
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.

5 participants