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

build: automate non-npm version updates #130

Closed
wants to merge 1 commit into from

Conversation

ObserverOfTime
Copy link
Contributor

This also applies to prereleases for maximum consistency.

sed -i Makefile -e 's/^VERSION := .*/VERSION := $(VERSION)/'
sed -i Cargo.toml -e 's/^version = .*/version = "$(VERSION)"/'
sed -i pyproject.toml -e 's/^version = .*/version = "$(VERSION)"/'
git add package.json package-lock.json Cargo.toml pyproject.toml Makefile
Copy link
Member

@justinmk justinmk Mar 27, 2024

Choose a reason for hiding this comment

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

Is this Makefile generated, or we have 100+ lines of Makefile code now?

Seems like these steps should be npm "scripts" that call node scripts, rather than introducing yet another tool (make) on top of the other tools.

Copy link
Member

Choose a reason for hiding this comment

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

This Makefile is generated. Upstream has started enhancing parser templates in a big way as part of a more integrated ecosystem.

Mid-term goal (work in progress) is to remove reliance on node and move more functionality into the tree-sitter CLI (so you can do tree-sitter release and get them all updated in sync).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this Makefile generated, or we have 100+ lines of Makefile code now?

Most of it is generated.

Seems like these steps should be npm "scripts" that call node scripts, rather than introducing yet another tool (make) on top of the other tools.

make was already a thing (for make parser/vimdoc.so) and it's more convenient to implement these here than in node.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, node is a pain for this kind of stuff.

Ok, my concerns are alleviated.

Copy link
Member

@clason clason Mar 27, 2024

Choose a reason for hiding this comment

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

However, these additions are in response to my declining to add node tooling just because I cannot figure out how to bump all versions consistently without messing up a git tag. I don't like these either since it pollutes the "C binding layer" with parser management.

Let's wait for upstream to figure this out properly; I'll try to remember to use --no-git-tag-version in the mean time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Makefile can be used for maintenance tasks as well (already had a test target).

Copy link
Member

@clason clason Mar 27, 2024

Choose a reason for hiding this comment

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

If this is part of the standard generated makefile (which I would advise against; if tree-sitter CLI is required for parser maintenance, one should use that and not offer ten different ways to do the same thing), we'll add it of course. (Can't promise I'll remember to use it, though; see above.)

What I object to is custom changes for this parser only for little gain (in relation to the cost of divergence). Especially if there's already a better approach in the works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is part of the standard generated makefile

It will be when tree-sitter/tree-sitter#3210 is ready (if the CLI command isn't implemented first).

(Can't promise I'll remember to use it, though; see above.)

You don't have to. npm version will call it automatically.

Copy link
Member

Choose a reason for hiding this comment

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

Then there's even less need for this PR ;)

Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

@clason any objections?

My Makefile comment is not a blocker but something to think about in the future. Lots of cruft accumulating.

@ObserverOfTime
Copy link
Contributor Author

The tree-sitter CLI will handle version updates in the future. This is a stopgap solution until then.

@clason clason closed this Mar 27, 2024
@ObserverOfTime ObserverOfTime deleted the update branch March 27, 2024 18:35
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.

None yet

3 participants