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-sys: use upstream packcc (take 2) #2947

Merged
merged 6 commits into from
Apr 9, 2021
Merged

Conversation

masatake
Copy link
Member

@masatake masatake commented Apr 7, 2021

This is based on #2491.
Instead of updating packcc.c, this change updates the upstream of 'git subtree'.

@masatake
Copy link
Member Author

masatake commented Apr 7, 2021

We cannot merge this till the Kotlin parser maintainer says "go".

@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #2947 (72fe40d) into master (6f06e29) will not change coverage.
The diff coverage is n/a.

❗ Current head 72fe40d differs from pull request most recent head f34fe2f. Consider uploading reports for the commit f34fe2f to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2947   +/-   ##
=======================================
  Coverage   87.24%   87.24%           
=======================================
  Files         194      194           
  Lines       44422    44422           
=======================================
  Hits        38755    38755           
  Misses       5667     5667           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f06e29...f34fe2f. Read the comment docs.

Copy link
Contributor

@dolik-rce dolik-rce left a comment

Choose a reason for hiding this comment

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

I have only two minor comments. The update looks good, even if you decide to not act on them.

@@ -6,4 +6,4 @@
# License: GPL 2 or later

cd $(git rev-parse --show-toplevel)
git subtree pull --prefix misc/packcc https://github.com/universal-ctags/packcc.git master --squash
git subtree pull --prefix misc/packcc https://github.com/arithy/packcc.git master --squash
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to pull in complete packcc repository? Most of it (test/, build/) won't be really useful for anything here?

Unfortunately, I think it is not possible to pull only some files using git subtree. On the other hand, you're probably not using git subtree with push or merge and you --squash the commits, so it's not really necessary to use it at all. You could simply pull the desired files using curl. It could look like this:

cd $(git rev-parse --show-toplevel)
SHA="$(curl 'https://api.github.com/repos/arithy/packcc/branches/master' | jq -r .commit.sha)"
for FILE in src/packcc.c LICENSE README.md; do
    mkdir -p "$(dirname "misc/packcc/$FILE")"
    curl "https://raw.githubusercontent.com/arithy/packcc/$SHA/$FILE" -o "misc/packcc/$FILE"
done
git commit -m "Updated packcc to $SHA" -- misc/packcc

Copy link
Member Author

Choose a reason for hiding this comment

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

I think getting the whole source tree is o.k.
We can avoid installing jq and curl for building ctags source tree if we use just git subtree.
We can expect the pull-packcc script works even if the directory layout of packcc is changed.
Of course, ctags' building script like Makefile.am must be revised when the layout is changed. However, we can reduce the items we have to think about when updating.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need curl and jq to build ctags, only if you want to update packcc. Or is the script called automatically somewhere in the build/release process? I thought you're running it manually only when you want to update.

But as I said, I don't really mind having more files here, you can leave this as is.

@@ -22,6 +22,10 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

I've made few more adjustments to the grammar. Could you please use the file from
https://github.com/dolik-rce/ctags/blob/35ea29c889b345946b3cd11bf3593b82de2de4eb/peg/kotlin.peg?

Copy link
Member Author

Choose a reason for hiding this comment

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

I put the new file to "build-sys: use upstream packcc" commit.
Is this o.k.?

We should separate the changes about kotlin.peg to another commit. However, when I removed the change about kotlin.peg from "build-sys: use upstream packcc" commit, I cannot build ctags. So we may have to put everything into the commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

packcc used in u-ctags was derived from enechaev/packcc.
I wonder the fixes in https://github.com/enechaev/packcc/commits/master are in the new upstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

I put the new file to "build-sys: use upstream packcc" commit.
Is this o.k.?

I don't mind. If you wan't it in two separate commits and both of them passing tests, then you'd have to first change the grammar and then add %earlysource later in the second commit.

I wonder the fixes in https://github.com/enechaev/packcc/commits/master are in the new upstream.

If you mean those 6 changes you listed in arithy/packcc#25, then yes, those are present in the new upstream. I'm not sure about other changes, but I think it was mostly features added by enechaev. As far as I can tell, everything works just fine with current state of this branch (I just checked again now).

Copy link
Member Author

Choose a reason for hiding this comment

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

I made a separate commit for changing about kotlin.peg. Thank you.

@masatake
Copy link
Member Author

masatake commented Apr 8, 2021

I added 7334d8c to recover the mistake I took in c1e350d.

dolik-rce and others added 6 commits April 9, 2021 05:01
Rearrange the grammer for new packcc.

(@masatake wrote this log.)
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
git-subtree-dir: misc/packcc
git-subtree-split: 4a7dbab60add4ed323ff7dd2caec7a1cb258b9f4
(@masatake added some minor changes to keep the source tree
 git bisect friendly.)
This may be cause an error when compiling generated .c files
with mvc.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@masatake masatake merged commit f6234d0 into master Apr 9, 2021
@masatake
Copy link
Member Author

masatake commented Apr 9, 2021

@dolik-rce, thank you very much.

@k-takata
Copy link
Member

@masatake Did you leave the switch-packcc-repo branch intentionally? Or can we delete the branch?
We also have some stale branches. Some of them might be deleted?
https://github.com/universal-ctags/ctags/branches/stale

We can also make our packcc repository archived.

@masatake masatake deleted the switch-packcc-repo branch April 11, 2021 10:10
@masatake
Copy link
Member Author

@k-takata, thank you. I deleted some of the obviously unnecessary branches. I converted universal-ctags/packcc repo to an archive.

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.

3 participants