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

pack: added TCM file packaging #1092

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

AlexandrLitkevich
Copy link

@AlexandrLitkevich AlexandrLitkevich commented Jan 27, 2025

pack: added TCM file packaging

Closes #TNTP-1097

@AlexandrLitkevich AlexandrLitkevich force-pushed the TNTP-1097v2 branch 3 times, most recently from b4b5da6 to 9f99d09 Compare January 27, 2025 09:24
@AlexandrLitkevich AlexandrLitkevich changed the title pack: Added TCM file packaging pack: added TCM file packaging Jan 27, 2025
@oleg-jukovec oleg-jukovec requested review from dmyger and removed request for oleg-jukovec January 27, 2025 12:07
@@ -104,6 +110,8 @@ type CliCtx struct {
Verbose bool
// TarantoolCli is current tarantool cli.
TarantoolCli TarantoolCli
// Tcmcli is current tcm cli.
TcmCli
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TcmCli
TcmCli TcmCli

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@patapenka-alexey patapenka-alexey left a comment

Choose a reason for hiding this comment

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

Thank you for the patch! LGTM.

Comment on lines 567 to 568
return "", fmt.Errorf(`found Tcm binary '%s' isn't executable: %s`,
localTcm, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be here need wrapping an error?

Suggested change
return "", fmt.Errorf(`found Tcm binary '%s' isn't executable: %s`,
localTcm, err)
return "", fmt.Errorf(`found Tcm binary '%s' isn't executable: %w`,
localTcm, err)


cmdCtx.Cli.TcmCli.Executable = localTcm
} else if !os.IsNotExist(err) {
return "", fmt.Errorf("failed to get access to Tcm binary file: %s", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrapping?

Suggested change
return "", fmt.Errorf("failed to get access to Tcm binary file: %s", err)
return "", fmt.Errorf("failed to get access to Tcm binary file: %w", err)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the comment.I took an example of the error above in the code

} else {
if err := util.CopyFileDeep(cmdCtx.Cli.TcmCli.Executable,
util.JoinPaths(pkgBin, "tcm")); err != nil {
return fmt.Errorf("failed copying tarantool: %s", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("failed copying tarantool: %s", err)
return fmt.Errorf("failed copying tarantool: %w", err)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the comment.I took an example of the error above in the code


if _, err := os.Stat(localTcm); err == nil {
if _, err := exec.LookPath(localTcm); err != nil {
return "", fmt.Errorf(`found Tcm binary '%s' isn't executable: %s`,
Copy link
Contributor

Choose a reason for hiding this comment

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

"%q" format do the quoting.

Suggested change
return "", fmt.Errorf(`found Tcm binary '%s' isn't executable: %s`,
return "", fmt.Errorf(`found Tcm binary %q isn't executable: %s`,

Copy link
Author

Choose a reason for hiding this comment

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

Done


cmdCtx.Cli.TcmCli.Executable = localTcm
} else if !os.IsNotExist(err) {
return "", fmt.Errorf("failed to get access to Tcm binary file: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return "", fmt.Errorf("failed to get access to Tcm binary file: %s", err)
return "", fmt.Errorf("failed to get access to TCM binary file: %s", err)
Suggested change
return "", fmt.Errorf("failed to get access to Tcm binary file: %s", err)
return "", fmt.Errorf("failed to get access to tcm binary file: %s", err)

Copy link
Author

Choose a reason for hiding this comment

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

Done

return "", fmt.Errorf("failed to get access to Tcm binary file: %s", err)
}

log.Debugf("tcm executable found: '%s'", cmdCtx.Cli.TcmCli.Executable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.Debugf("tcm executable found: '%s'", cmdCtx.Cli.TcmCli.Executable)
log.Debugf("tcm executable found: %q", cmdCtx.Cli.TcmCli.Executable)

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines +270 to +271
// Copy tcm.
if packCtx.WithBinaries {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add test cases into Test_prepareBundle in the common_test.go.

@@ -787,6 +787,7 @@ def test_pack_tgz_compat_with_binaries(tt_cmd, tmp_path):

assert os.path.isfile(os.path.join(app_path, "tt"))
assert os.path.isfile(os.path.join(app_path, "tarantool"))
assert os.path.isfile(os.path.join(app_path, "tcm"))
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I don't understand where this file came from? I don't see any "tcm" file is created.

Copy link
Author

Choose a reason for hiding this comment

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

I cleared it, this line is superfluous here.Verification added on line 1017

@oleg-jukovec oleg-jukovec added the full-ci Enables full ci tests label Feb 2, 2025
CHANGELOG.md Outdated
@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

### Added

- `tt pack `: added TCM file packaging
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `tt pack `: added TCM file packaging
- `tt pack `: added TCM file packaging.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@AlexandrLitkevich AlexandrLitkevich force-pushed the TNTP-1097v2 branch 2 times, most recently from ac8f675 to 10af852 Compare February 3, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables full ci tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants