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

chore: add version updating script in preparation for auto-release CI #16

Merged
merged 2 commits into from
Jul 20, 2022

Conversation

jvanstraten
Copy link
Collaborator

No description provided.

template = f.read()

# Fill out the template.
patch = template.format(frm=current_version, to=new_version).encode("utf-8")
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 something like cargo bump instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't aware of it's existence, but...

thread 'main' panicked at 'Workspaces are not supported yet.'

Also, it still wouldn't bump the version information for the Python bindings, in documentation, etc.

Copy link
Contributor

@cpcloud cpcloud Jun 23, 2022

Choose a reason for hiding this comment

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

Right, but that should all be handled using the tools to do such tasks for each ecosystem, or with a replacement tool such as https://github.com/google/semantic-release-replace-plugin.

For cargo bump it seems like you can handle its lack of workspace support by listing the crates and looping over them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also tried from the subdirectories; it still fails. Also the issue tracking this has been stale for 3 1/2 years. In fact, all of cargo bump has been stale for 3 years. It's not exactly a standard component of cargo.

I'm perfectly fine with replacing the script with that semantic release plugin, in which case this PR can just be closed. Though to be honest it seems like a lot of complexity for what looks to be a regex replacement, which in itself seems more likely to break than a patchfile... Anyway, do you have time to help me hammer out the semantic release logic sometime, or at least to get me started with it? I remember being very confused about the logic on the main repo.

with open("version", "r") as f:
current_version = f.read().strip()

if len(sys.argv) == 2 and sys.argv[1] == "get":
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use argparse for all of this argument handling. It handles subcommands and will make the handling much easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough. Was just being lazy to be honest.

skip = True
num_changes += 1
else:
template_lines.append(escape(line))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is all of this code doing something other than bumping the crates in the workspace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See https://github.com/substrait-io/substrait-validator/blob/main/RELEASE.md#incrementing-version-numbers. tl;dr, various duplicates of the version number in the various cargo.toml files, a reference in Rust's readme/docs for the customary copypasta to include a library as a dependency, and importantly pyproject.toml. It also helps update and sanity-check the patch file, which would be a massive pain otherwise.

Note that this is not exactly my idea of a clean solution, but (Python script implementation aside) I'm not sure how to do it better without making the whole thing massively more complex by special-casing everything or generating all these files.

print()
print(f"{me} update")
print(" Interactively updates the patchfile template used to update")
print(" the version")
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this can be specified using argparse.

@jvanstraten jvanstraten merged commit e8733c0 into substrait-io:main Jul 20, 2022
@jvanstraten jvanstraten deleted the version-update-script branch September 8, 2022 07:42
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.

2 participants