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 mill examples in init #3583

Merged
merged 47 commits into from
Sep 30, 2024
Merged

Use mill examples in init #3583

merged 47 commits into from
Sep 30, 2024

Conversation

pawelsadlo
Copy link
Contributor

@pawelsadlo pawelsadlo commented Sep 20, 2024

implements #3548
Modifying init to fetch examples from releases page instead of using g8 template

This introduces new top-level module initmodule
this module generates resource exampleList.txt containing json with array of pairs (exampleId,exampleUrl)
exampleId and exampleUrl are formed based on already present exampleZips target, and millVersion().

initmodule introduces external module MillInitModule
calls tomill init are redirected to MillInitModule.init (basically the same logic as it was with Giter8Module.init)

MillInitModule.init called without parameters prints message containing exampleids list based on generated exampleList.txt in the form of:

Run init with one of the following examples as an argument to download and extract example:
depth/cross/1-simple
depth/cross/2-cross-source-path
...

When called with a parameter it validates if passed parameter is in the list, and if so, it downloads example from github releases page and unpacks it into the directory from where the command was run.

@pawelsadlo2
Copy link

@lihaoyi I have added some integration tests which tries to download examples from GitHub repo,
They are working locally but failing on Ci, i assume that CI can't access internet right?

@lihaoyi
Copy link
Member

lihaoyi commented Sep 21, 2024

CI should be able to access the internet, not sure why it's failing

@pawelsadlo
Copy link
Contributor Author

pawelsadlo commented Sep 21, 2024

I have found the issue, I was relying on millVersion target to build download URLs.

But nor millVersion, neither millLastTag targets returns valid versions when runned on CI.

@lihaoyi
Is there any reliable function that return last released mill version already implemented in codebase?

@lihaoyi
Copy link
Member

lihaoyi commented Sep 22, 2024

@pawelsadlo I think for the purposes of this ticket, let's skip end-to-end tests that download from Github. Those are inherently non-hermetic and we don't have the infrastructure to stand up test github instances to make them hermetic. Smaller unit tests for the various pieces of logic, maybe an integration test with the download-zip-from-github logic overriden with a local file, and manually exercising the end to end workflow, should be enough for now

@pawelsadlo pawelsadlo marked this pull request as ready for review September 23, 2024 08:07
@pawelsadlo
Copy link
Contributor Author

@lihaoyi, could you review, please?

@lihaoyi
Copy link
Member

lihaoyi commented Sep 23, 2024

@pawelsadlo could you update the PR description with a summary of the changes and anything worth taking note of? That would make the review easier

@pawelsadlo
Copy link
Contributor Author

@lihaoyi I have updated the description

@lihaoyi
Copy link
Member

lihaoyi commented Sep 23, 2024

Thanks! will take a look tomorrow

@pawelsadlo
Copy link
Contributor Author

@lihaoyi
Ad1. done
Ad2 -
it extracts into subfolder - this exception is thrown if there is already an existing subfolder with the same name
g8 also extracts to subfolder, however it merges folders recursively if there is already existing one

do you have any better error msg in mind?

will add test shortly.

Ad3 - its working as expected, I just didnt update repo locally.

@pawelsadlo
Copy link
Contributor Author

pawelsadlo commented Sep 27, 2024

@lihaoyi
I have implemented every suggestion, however there is a problem

link to example is of the form:
<millRepo>/releases/download/<millLastTag()>/<millVersion()>-example-depth-cross-2-cross-source-path.zip

but <millLastTag()> is failing on CI

so what is coded now is millVersion() instead of millLastTag()
<millRepo>/releases/download/<millVersion()>/<millVersion()>-example-depth-cross-2-cross-source-path.zip

but this forms malformed links for current version
<millRepo>/releases/download/0.12.0-RC2-38-029986/0.12.0-RC2-38-029986-example-depth-cross-2-cross-source-path.zip

instead of:
<millRepo>/releases/download/0.12.0-RC2/0.12.0-RC2-38-029986-example-depth-cross-2-cross-source-path.zip

Is this a big problem to have it wrong in RC version (assuming that in release it will be fixed)?
is there any tool to know that mill is being build on CI?

if its a big problem we can always manually get tag from millVersion() by parsing the string - it should be quite easy to do (this would remove millLastTag() call, which is failing the CI)

Edit: actually i think catching error on millLastTag , and emiting warning both to the console and writing it instead of exampleUrl would be the best if we can't detect running on CI, what do you think?

@lihaoyi
Copy link
Member

lihaoyi commented Sep 27, 2024

@pawelsadlo I'd be OK parsing millVersion(). I'm a bit curious how come millLastTag fails in CI when millVersion() doesn't, as I would expect both to end up calling the same underlying Git APIs. Maybe @lefou knows?

Mill on Github Actions should have the CI=true environment variable set, according to the docs https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables, so we should be able to check for that as well

@pawelsadlo
Copy link
Contributor Author

@pawelsadlo I'd be OK parsing millVersion(). I'm a bit curious how come millLastTag fails in CI when millVersion() doesn't, as I would expect both to end up calling the same underlying Git APIs. Maybe @lefou knows?

There is just a hard coded fallback to 0.0.0 on mill version targer.

Mill on Github Actions should have the CI=true environment variable set, according to the docs https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables, so we should be able to check for that as well

I was trying this, but it didn't work.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 28, 2024

Thanks @pawelsadlo, give me some time to tinker around with the PR and I think we can merge it.

@pawelsadlo
Copy link
Contributor Author

Sure , take your time

@lihaoyi
Copy link
Member

lihaoyi commented Sep 30, 2024

Looks good to me, i'll continue tinkering with it post-merge. @pawelsadlo will send the bank transfer using the same details as before

@lihaoyi lihaoyi merged commit 5e51a3b into com-lihaoyi:main Sep 30, 2024
24 checks passed
@pawelsadlo
Copy link
Contributor Author

pawelsadlo commented Sep 30, 2024

@lihaoyi
I have added one more PR taking milestones into account when parsing millLastTag from millVersion #3625

@lefou lefou added this to the 0.12.0-RC3 milestone Sep 30, 2024
@bishabosha
Copy link
Contributor

was there a specific reason why the codesig object was moved to its own package.mill?

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