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

Use cut instead of $CUT #490

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Conversation

RadekCap
Copy link
Contributor

Add check of CUT.

CUT is needed for a proper setting of LIB_DIR in CYGWIN.

Add check of CUT.

CUT is needed for a proper setting of LIB_DIR in CYGWIN.
@RadekCap RadekCap changed the title Update makefile Add check of CUT Jan 23, 2024
@sophia-guo
Copy link
Contributor

cut is only use once for CYGWIN check UNAME_OS := $(shell $(UNAME) -s | $(CUT) -f1 -d_). May just use cut instead.

@judovana
Copy link
Contributor

@sophia-guo can you please elaborate more? Do yo recomend to use cut instead $(CUT) ? Whats the benefit? The cut will still be missing from path. And it will enad as command not found...

I'm missing some detection of the cut. Where do you expect it to be set? Can you point us?

@judovana
Copy link
Contributor

@RadekCap maybe you can add fallback?

if $(CUT) is missing, maybe yo may check path for cut it, and if it is there, then use that, if not then indeed die...

@sophia-guo
Copy link
Contributor

I mean use cut directly there is no need to define CUT as it is used only once and presumably that will be true for a long time.

@RadekCap and I have talked about it in a meeting and @RadekCap will update the PR accordingly.

use cut instead of $CUT
Add debug message about used LIB_DIR.
@RadekCap
Copy link
Contributor Author

@sophia-guo updated as requested.

I've added a debug message for LIB_DIR as well.

@RadekCap RadekCap changed the title Add check of CUT Use cut instead of $CUT Jan 30, 2024
@judovana
Copy link
Contributor

I think the verification that the cut is on path should be done. Also i think that calling cut without $(CUT) is bad code.

@sophia-guo
Copy link
Contributor

sophia-guo commented Jan 30, 2024

I think the cut command comes pre-installed in most Linux distributions. Though agreed @judovana it doesn't hurt to verify.

@sophia-guo sophia-guo merged commit 6ab3f8f into adoptium:master Feb 1, 2024
3 checks passed
@judovana
Copy link
Contributor

judovana commented Feb 3, 2024

-UNAME_OS := $(shell $(UNAME) -s | $(CUT) -f1 -d_)
+UNAME_OS := $(shell $(UNAME) -s | cut -f1 -d_)

Just to summ this up - we agreed it should be checked - but the patch was approvred without the check.
In addition we caused (microscopical) regression, that if I have CUT declared, it will be ignored.

Am I right or am reading it wrong?

I know the proper gues/set/check if is nto exactly plesant, but Radek's original commit was very close to it. Also I agree it is super minor thing, but I feel very very uncomfortable about it:( Do we agree on finish it properly (@RadekCap please?), or do you really want to set this precedent?

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.

4 participants