Skip to content

Adds .fromGit and buildLocal #8

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 15 commits into from
Mar 10, 2025
Merged

Conversation

MichealReed
Copy link
Contributor

@MichealReed MichealReed commented Mar 8, 2025

This adds a new factory fromGit and a flag that can be passed to both factories buildLocal. Had to manually init the directory instead of clone it so that we have control over the name of the directory the sources are pulled to.

Not sure how you want to split the example, added line comments to the add project's build hook for now.

I looked at adding a test but there are currently many failures with the defaults, these are mostly imported from the _c library?

closes #5

@rainyl
Copy link
Owner

rainyl commented Mar 9, 2025

@MichealReed Thanks! I have reviewed it.

Not sure how you want to split the example, added line comments to the add project's build hook for now.

Okay, adding a new example like libadd_from_git inside example dir is fine.

I looked at adding a test but there are currently many failures with the defaults, these are mostly imported from the _c library?

You can add a test and I can offer some help to figure it out 😄

@MichealReed
Copy link
Contributor Author

@MichealReed Thanks! I have reviewed it.

Not sure how you want to split the example, added line comments to the add project's build hook for now.

Okay, adding a new example like libadd_from_git inside example dir is fine.

I looked at adding a test but there are currently many failures with the defaults, these are mostly imported from the _c library?

You can add a test and I can offer some help to figure it out 😄

If both the dart_cli and flutter examples are wired to use add, I do not see how the example could be used by adding add_from_git. Maybe it's better to demonstrate these from the same hook with a bool?

@rainyl
Copy link
Owner

rainyl commented Mar 9, 2025

If both the dart_cli and flutter examples are wired to use add, I do not see how the example could be used by adding add_from_git. Maybe it's better to demonstrate these from the same hook with a bool?

just create a new example named libadd_from_git or something else is okay for now, it's just an example showing how to use the package, with dart-lang/native#39 landed, it will be easier to switch between different constructors.

@MichealReed
Copy link
Contributor Author

If both the dart_cli and flutter examples are wired to use add, I do not see how the example could be used by adding add_from_git. Maybe it's better to demonstrate these from the same hook with a bool?

just create a new example named libadd_from_git or something else is okay for now, it's just an example showing how to use the package, with dart-lang/native#39 landed, it will be easier to switch between different constructors.

I added a runBuildGit to the hook helpers and

    const exampleGit = false;
    if (!exampleGit) {
      await runBuild(input, output, sourceDir);
    } else {
      await runBuildGit(input, output, sourceDir);
    }

Can we use this approach instead?

@rainyl
Copy link
Owner

rainyl commented Mar 9, 2025

I added a runBuildGit to the hook helpers and

    const exampleGit = false;
    if (!exampleGit) {
      await runBuild(input, output, sourceDir);
    } else {
      await runBuildGit(input, output, sourceDir);
    }

Can we use this approach instead?

Sure, it's clearer, great jobs.

@MichealReed
Copy link
Contributor Author

Sure, it's clearer, great jobs.

Thanks! Tests added to builder/cmake_builder_git_test.dart too. Good to merge?

@rainyl
Copy link
Owner

rainyl commented Mar 9, 2025

Thanks! Tests added to builder/cmake_builder_git_test.dart too. Good to merge?

I am facing a problem when testing.

image

As you can see, some users like me are still using master as default branch name, and if gitBranch is different, it will be failed.

So maybe we should fetch first and checkout to gitBranch or gitCommit?

Also, Process.runSync used in the constructor only logs stdout, but errors are not correctly processed, so I added runProcessSync() to do this, could you please finish the rest since it's late here, thanks~

Please let me know if you do not like runProcessSync or my other changes.

@MichealReed
Copy link
Contributor Author

Thanks! Tests added to builder/cmake_builder_git_test.dart too. Good to merge?

I am facing a problem when testing.

image

As you can see, some users like me are still using master as default branch name, and if gitBranch is different, it will be failed.

So maybe we should fetch first and checkout to gitBranch or gitCommit?

Also, Process.runSync used in the constructor only logs stdout, but errors are not correctly processed, so I added runProcessSync() to do this, could you please finish the rest since it's late here, thanks~

Please let me know if you do not like runProcessSync or my other changes.

Ah I think this is why we need to use fetch. Will mess around with this in a few hours.

@MichealReed
Copy link
Contributor Author

MichealReed commented Mar 9, 2025

Ok, this should work we use --

mkdir test
git init
git remote add origin gitURL
git fetch origin gitBranch
git reset --hard gitCommit or FETCH_HEAD (default)

@rainyl
Copy link
Owner

rainyl commented Mar 10, 2025

Ok, this should work we use --

mkdir test git init git remote add origin gitURL git fetch origin gitBranch git reset --hard gitCommit or FETCH_HEAD (default)

Great jobs! I have made some small changes and improved testing, now LGTM.

I will merge it if it's also looks good to you.

@MichealReed
Copy link
Contributor Author

Looks good to me, thanks for the improvements!

@rainyl rainyl merged commit e59096d into rainyl:main Mar 10, 2025
10 checks passed
@rainyl
Copy link
Owner

rainyl commented Mar 10, 2025

@MichealReed Thanks for your contributions! 😄

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.

createFromGit factory helper
2 participants