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

WIP: CHG: Use pycparser as a git submodule #159

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AlexLao512
Copy link
Contributor

WIP: Needs more testing.

Use pycparser as submodule.

@JulianKemmerer
Copy link
Owner

IIUC this does not pin a version of pycparser - and would, as coded now, have people doing git init recursive clone stuff and would pull the latest pycparser code
@Datavenia does this work with the direction/mechanism you were thinking for pinning versions?

@ghost
Copy link

ghost commented Feb 5, 2023

versions?

We could add a pinned version to the pyproject.toml, this means people would need to use poetry as a user but it's probably the most extensible system.

@AlexLao512
Copy link
Contributor Author

AlexLao512 commented Feb 5, 2023

IIUC this does not pin a version of pycparser - and would, as coded now, have people doing git init recursive clone stuff and would pull the latest pycparser code @Datavenia does this work with the direction/mechanism you were thinking for pinning versions?

It actually does pin, there is a .gitmodules file with the URL to the repository with a release tag (version).

@AlexLao512
Copy link
Contributor Author

AlexLao512 commented Feb 5, 2023

Sorry, some mistakes above, the commit on the remote repo is pinned here. If you click through to the commit you will see the release tag.

image

image

If is in the submodule itself via commit hash and not in the .gitmodules branch = because the remote repo has a TAG on master, instead of a release branch and tag.

@AlexLao512
Copy link
Contributor Author

versions?

We could add a pinned version to the pyproject.toml, this means people would need to use poetry as a user but it's probably the most extensible system.

I suggest we use a setup.py so pip can be used. pip is installed along with the users Python install, no additional pieces needed on the users end.

@JulianKemmerer
Copy link
Owner

It sounds like doing like this PR doing, and using git submodules to pull in pycparser at a certain commit point ~essentially having a copy of the repo inside pipelinec repo like now but the support git way - makes sense to do. I of course want to test it but sounds simple and easy.

And then after, in a separate PR/issue perhaps, is the question of 'proper' dependency/package management for python projects? And IIUC it has to be either or pip or poetry based management? Idk if it can be both easily?

But I am thinking this PR can go through sooner than deciding on / setting up pip/poetry with pycparser dependency etc

@AlexLao512
Copy link
Contributor Author

Yea so this does half the cleanup required for moving to proper Python package management, the submodule part is trivial to remove later. If you want to test, I think this can be merged.

@JulianKemmerer
Copy link
Owner

Yeah testing I want to do is basic - does it still seem to parse c code and not crash kinda tests

Should have some time this week

@JulianKemmerer
Copy link
Owner

Wow I just realized you got the pycparser 2.18 tagged commit version that should exactly match what I had copied into the repo - tests should go smoothly 👍

@JulianKemmerer
Copy link
Owner

IIUC

New repo clone will need to be git clone --recurse-submodules

And users pulling to update their repo need to one time update+init submodules

git pull
git submodule update --init --recursive

@JulianKemmerer
Copy link
Owner

@AlexLao512 can you change the temp work around locations in code to use the absolute path to the repo?

from utilities import REPO_ABS_DIR

# TODO: Temporarily import from submodule, remove this hack when we create a proper pipelinec setup.py
sys.path.append(REPO_ABS_DIR()+'/submodule/pycparser')

Otherwise depends on being in ./src/ as is on branch


If I wanted to make/suggest this change I would need to clone your fork of my repo w/ the branch - and then make another branch to act as a PR into your branch? 😬

@JulianKemmerer
Copy link
Owner

JulianKemmerer commented Feb 22, 2023

Oh dope - I didnt know I could just commit and push to a repo owned by not my user (I guess forking my repo gives me access by default idk?)

So I think I can continue to make the changes need to the PR for full merge 👍 hopefully sooner than later...

@JulianKemmerer
Copy link
Owner

Something fucky is up

Trying the branch I get a C parsing related ~'nodes in a dictionary arent as expected' internal pipelinec sanity check error (tried examples/blink.c)

Weird...

C_AST_VAL_UNIQUE_KEY_DICT_MERGE Dicts aren't unique: {'BIN_OP_EQ[blink_c_l17_c6_e220]': <pycparser.c_ast.BinaryOp object at 0x7f341b2f3310>, 'TRUE_CLOCK_ENABLE_MUX[blink_c_l18_c1_af86]': <pycparser.c_ast.Compound object at 0x7f341b2e8180>, 'FALSE_CLOCK_ENABLE_MUX[blink_c_l25_c1_5b0e]': <pycparser.c_ast.Compound object at 0x7f341b2e8040>, 'UNARY_OP_NOT[blink_c_l20_c12_cefe]': <pycparser.c_ast.UnaryOp object at 0x7f341b4baa00>} {'BIN_OP_EQ[blink_c_l17_c6_e220]': <pycparser.c_ast.BinaryOp object at 0x7f341b2f3220>, 'TRUE_CLOCK_ENABLE_MUX[blink_c_l18_c1_af86]': <pycparser.c_ast.Compound object at 0x7f341b1cc7c0>, 'FALSE_CLOCK_ENABLE_MUX[blink_c_l25_c1_5b0e]': <pycparser.c_ast.Compound object at 0x7f341b1ccc40>, 'BIN_OP_PLUS[blink_c_l26_c5_c986]': <pycparser.c_ast.BinaryOp object at 0x7f341b2f3630>}
key BIN_OP_EQ[blink_c_l17_c6_e220]
v1 <pycparser.c_ast.BinaryOp object at 0x7f341b2f3310> blink_c_l17_c6_5b93
v2 <pycparser.c_ast.BinaryOp object at 0x7f341b2f3220> blink_c_l17_c6_72f9
Traceback (most recent call last):
  File "./src/pipelinec", line 109, in <module>
    parser_state = C_TO_LOGIC.PARSE_FILE(c_file)
  File "/home/julian/Desktop/AlexLao512/PipelineC/src/C_TO_LOGIC.py", line 9723, in PARSE_FILE
    parser_state.FuncLogicLookupTable = APPEND_FUNC_NAME_LOGIC_LOOKUP_TABLE(
  File "/home/julian/Desktop/AlexLao512/PipelineC/src/C_TO_LOGIC.py", line 10967, in APPEND_FUNC_NAME_LOGIC_LOOKUP_TABLE
    logic = C_AST_FUNC_DEF_TO_LOGIC(
  File "/home/julian/Desktop/AlexLao512/PipelineC/src/C_TO_LOGIC.py", line 8807, in C_AST_FUNC_DEF_TO_LOGIC
    body_logic = C_AST_NODE_TO_LOGIC(
  File "/home/julian/Desktop/AlexLao512/PipelineC/src/C_TO_LOGIC.py", line 1901, in C_AST_NODE_TO_LOGIC
    return C_AST_COMPOUND_TO_LOGIC(c_ast_node, prepend_text, parser_state)
  File "/home/julian/Desktop/AlexLao512/PipelineC/src/C_TO_LOGIC.py", line 5188, in C_AST_COMPOUND_TO_LOGIC
    item_logic = C_AST_NODE_TO_LOGIC(
  File "/home/julian/Desktop/AlexLao512/PipelineC/src/C_TO_LOGIC.py", line 1905, in C_AST_NODE_TO_LOGIC
    return C_AST_IF_TO_LOGIC(c_ast_node, prepend_text, parser_state)
  File "/home/julian/Desktop/AlexLao512/PipelineC/src/C_TO_LOGIC.py", line 5997, in C_AST_IF_TO_LOGIC
    true_logic.MERGE_COMB_LOGIC(false_logic)
  File "/home/julian/Desktop/AlexLao512/PipelineC/src/C_TO_LOGIC.py", line 762, in MERGE_COMB_LOGIC
    self.submodule_instance_to_c_ast_node = C_AST_VAL_UNIQUE_KEY_DICT_MERGE(
  File "/home/julian/Desktop/AlexLao512/PipelineC/src/C_TO_LOGIC.py", line 315, in C_AST_VAL_UNIQUE_KEY_DICT_MERGE

@AlexLao512
Copy link
Contributor Author

Might be a good idea to diff the version you have in the repo and the tagged releases of pycparser.

@JulianKemmerer
Copy link
Owner

Hopefully I can get around to this in the next week or so 👍 , starting with the diff

@JulianKemmerer JulianKemmerer self-assigned this Apr 26, 2023
@JulianKemmerer
Copy link
Owner

JulianKemmerer commented Apr 27, 2023

OK I think I know whats going on.

The version of the files I copied into my repo in ~2017/2018 some time was marked at version 2.18. And that is what was used for the submodule.

However, the files are marked as version 2.18 all the way through to 2.19. Meaning, I likely had copied the files after 2.18 release, including any differences accumulated since but before 2.19.

Anywhoo
if you look here
https://github.com/eliben/pycparser/commits/caa4c11ebb99ed5cf854dc6342b5352d5ff52686

The commit right before 2.19 is eliben/pycparser@bcb5f9f

I checked out that commit on top of this PR and it seems to be working fine in early tests...

@JulianKemmerer
Copy link
Owner

Perhaps it would be sane to try a version of this PR using exactly version 2.19 which differs by only the version bump commit 😅

@JulianKemmerer
Copy link
Owner

Now thats the submodule is up to 2.19 again the last thing seems to be fixing the path append like

from utilities import REPO_ABS_DIR

# TODO: Temporarily import from submodule, remove this hack when we create a proper pipelinec setup.py
sys.path.append(REPO_ABS_DIR() + '/submodule/pycparser')

@JulianKemmerer
Copy link
Owner

Looking great so far, a test build running...

But I pushed another minor fix to the pipelinec main repo

How do I get that change onto this branch under test - you mentioned 'rebase' is what I want? - ill do some googling 😏

@AlexLao512
Copy link
Contributor Author

Done. You don't actually have push access to my fork. I can pull changes from your forks master branch into my forks master branch, then I can rebase my forks branch onto my forks master branch (which is now in sync with yours).

@JulianKemmerer
Copy link
Owner

I have more testing to do than I anticipated

Using the new pycparser version I am getting designs with ~10% more LUTs

Trying to decide how to debug this or just accept the hit for now...

@JulianKemmerer JulianKemmerer removed their assignment May 10, 2023
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

2 participants