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

sparse checkout feature - Load the package after downloading the entire repository #452

Conversation

officialasishkumar
Copy link
Contributor

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y

2. What is the scope of this PR (e.g. component or file name):

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

Signed-off-by: Asish Kumar <[email protected]>
@officialasishkumar
Copy link
Contributor Author

PTAL @zong-zhe @Peefy

@Peefy
Copy link
Contributor

Peefy commented Aug 14, 2024

cc @zong-zhe Could you help review it?

@officialasishkumar
Copy link
Contributor Author

officialasishkumar commented Aug 14, 2024

By mistake i defined the package in https://github.com/kcl-lang/kcl-go/blob/1460c5ca64b0d9ef3c10d7a7b1dcc7f98991bd87/pkg/kcl/opt.go#L15 and it worked locally. I didn't realised that and pushed it. I was wondering do i have to make changes to that repo too?

There is another option to pass package here https://github.com/kcl-lang/kcl-go/blob/1460c5ca64b0d9ef3c10d7a7b1dcc7f98991bd87/pkg/spec/gpyrpc/gpyrpc.pb.go#L1497

lmk which sounds good

cc: @Peefy @zong-zhe

@zong-zhe
Copy link
Contributor

By mistake i defined the package in https://github.com/kcl-lang/kcl-go/blob/1460c5ca64b0d9ef3c10d7a7b1dcc7f98991bd87/pkg/kcl/opt.go#L15 and it worked locally. I didn't realised that and pushed it. I was wondering do i have to make changes to that repo too?

There is another option to pass package here https://github.com/kcl-lang/kcl-go/blob/1460c5ca64b0d9ef3c10d7a7b1dcc7f98991bd87/pkg/spec/gpyrpc/gpyrpc.pb.go#L1497

lmk which sounds good

cc: @Peefy @zong-zhe

Hi @officialasishkumar 😄

At present, define the package field in the kpm project because the current definition of KCL-GO is the go-sdk of KCL language; from the language side, the compiler can not understand the concept of package; all package-related content should be managed by kpm. Although they all work well, having a clear division of responsibility for each project is more helpful.

Signed-off-by: Asish Kumar <[email protected]>
@officialasishkumar
Copy link
Contributor Author

Hi @zong-zhe ,

Thanks for the response. I have changed the code accordingly. Please have a look : )

@coveralls
Copy link

coveralls commented Aug 15, 2024

Pull Request Test Coverage Report for Build 10433442117

Details

  • 1 of 53 (1.89%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 40.105%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/client/client.go 1 9 11.11%
pkg/cmd/cmd_run.go 0 8 0.0%
pkg/utils/utils.go 0 36 0.0%
Totals Coverage Status
Change from base Build 10383495256: -0.3%
Covered Lines: 3129
Relevant Lines: 7802

💛 - Coveralls

pkg/utils/utils.go Outdated Show resolved Hide resolved
}

var modFile ModFile
_, err := toml.DecodeFile(kclModPath, &modFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the loadpackage function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is it? Can you share the link of the code or the docs?

@@ -599,3 +600,48 @@ func AbsTarPath(tarPath string) (string, error) {

return absTarPath, nil
}

func FindPackage(root, targetPackage string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add more documents and unit tests?

return err
}
if info.IsDir() {
kclModPath := filepath.Join(path, "kcl.mod")
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the kcl.mod constant defined in the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@Peefy
Copy link
Contributor

Peefy commented Aug 17, 2024

cc @zong-zhe Could you help review it?

@officialasishkumar I am not sure if this PR can meet all the requirements of LFX, but you should at least provide complete PR information for the functionality according to your changed code. Therefore, I only left some comments on the content of this PR.

However, I believe that there is still a lot of work to be done to complete the entire topic and enable users to add Git dependent submodules using kcl mod add.

@Peefy
Copy link
Contributor

Peefy commented Aug 17, 2024

#335

Additionally, the PR for this plan has been four months and it has still not been merged... cc @Zongzhe @officialasishkumar

@officialasishkumar
Copy link
Contributor Author

officialasishkumar commented Aug 17, 2024

Thanks for the review. Will look into it!

I am not sure if this PR can meet all the requirements of LFX,

Could you please clarify all the requirements so that I can start working on it?

cc: @Peefy @zong-zhe

@Peefy
Copy link
Contributor

Peefy commented Aug 17, 2024

@officialasishkumar

The two most important things:

  1. The design document is correct and merged
  2. Develop corresponding features and tests, merge this PR, users can use kcl mod add <sub_package> --git foo or set sub package in the kcl.mod file to add sub modules for a git dependency.

The problem I currently see is that your design doc and code do not match, and I believe you should first design the correct solution.

cc @zong-zhe

@officialasishkumar
Copy link
Contributor Author

@Peefy

The implementation of my design docs was in #387

@Peefy
Copy link
Contributor

Peefy commented Aug 17, 2024

@Peefy

The implementation of my design docs was in #387

Can you provide detailed information on the association between PR and Issue? Why is it confusing that all your PR titles are the same?

@officialasishkumar officialasishkumar changed the title sparse checkout feature sparse checkout feature - Load the package after downloading the entire repository Aug 17, 2024
@officialasishkumar
Copy link
Contributor Author

Sure thing, I have updated the title.

kcl mod add <sub_package> --git foo

Since the idea was changed that is to download the entire repo, I asked in slack whether to use package command in add or run. I found run to be more suitable since the repo is download entirely and the user can change it whenever they want while running their program. So, I implemented it the same way.

@Peefy
Copy link
Contributor

Peefy commented Aug 17, 2024

I think you should update design in this PR #335 and merge it firstly. I don't think user interfaces are right in this doc.

@officialasishkumar
Copy link
Contributor Author

officialasishkumar commented Aug 17, 2024

@Peefy
Okay, So which idea should I write down in the design doc?

kcl mod run --package test

or

kcl mod add --git <git-url> --commit <commit-hash> --package test

Will also put the package name in kcl.mod file.

I will update the code accordingly.

@Peefy
Copy link
Contributor

Peefy commented Aug 20, 2024

#453 is merged. Do we still need to keep this PR? @officialasishkumar

@officialasishkumar
Copy link
Contributor Author

No @Peefy

@Peefy Peefy closed this Aug 22, 2024
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.

Enhancement: kpm add command displaying optimization
4 participants