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

Fix: ar variable in Makefile to inherit upstream ar command #217

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

gregnr
Copy link
Contributor

@gregnr gregnr commented Nov 3, 2023

Fantastic library!

I'm working on compiling this library to WASM using Emscripten (specifically for libpg-query-node). Emscripten has a command emmake that simply wraps make with CC, CXX, AR, etc set to the Emscripten drop-in replacements (emcc, em++, emar).

Surprisingly running emmake make on this repository's Makefile JustWorked™ with the exception of a one-line change to the Makefile which is what this PR proposes.

We just need to replace the hardcoded AR variable:

AR = ar rs

with a reference to the upstream inherited AR variable:

AR := $(AR) rs

so that Emscripten's emar replacement is used.

Let me know if you have any questions or need any more clarification.

Makefile Show resolved Hide resolved
@lfittl
Copy link
Member

lfittl commented Nov 3, 2023

@gregnr Thanks for the contribution! I think that looks good, with one minor question inline.

@gregnr
Copy link
Contributor Author

gregnr commented Nov 3, 2023

@lfittl thanks for the quick review! I've pushed an update based on your suggestion.

PS. are you open to applying this change retroactively to v13 and v14 branches for backwards support? If so I can create 2 more PR's for those.

@lfittl lfittl merged commit 0a112f6 into pganalyze:15-latest Nov 6, 2023
8 checks passed
@lfittl
Copy link
Member

lfittl commented Nov 6, 2023

PS. are you open to applying this change retroactively to v13 and v14 branches for backwards support? If so I can create 2 more PR's for those.

We generally don't back patch, unless there is a good reason to do so. But not against it per se. Do you have a use case that requires using older parser versions?

@gregnr
Copy link
Contributor Author

gregnr commented Nov 7, 2023

Thanks for merging!

Do you have a use case that requires using older parser versions?

Yes - I was planning to support v13 and v14 WASM builds as well (when paired with older Postgres versions), for which I would reference the respective version tags on this repo (v13, v14). But they will need the same Makefile patch to work.

@lfittl
Copy link
Member

lfittl commented Nov 7, 2023

Do you have a use case that requires using older parser versions?

Yes - I was planning to support v13 and v14 WASM builds as well (when paired with older Postgres versions), for which I would reference the respective version tags on this repo (v13, v14). But they will need the same Makefile patch to work.

Ack, makes sense. I think we can support that, if you want to make a PR for them :)

@gregnr
Copy link
Contributor Author

gregnr commented Feb 16, 2024

Ack, makes sense. I think we can support that, if you want to make a PR for them :)

I finally got around to doing this. Here are the PR's for v13 and v14:

Thanks @lfittl!

@lfittl
Copy link
Member

lfittl commented Feb 17, 2024

Ack, makes sense. I think we can support that, if you want to make a PR for them :)

I finally got around to doing this. Here are the PR's for v13 and v14:

* **v13:** [Fix: ar variable in Makefile (back patch to v13) #239](https://github.com/pganalyze/libpg_query/pull/239)

* **v14:** [Fix: ar variable in Makefile (back patch to v14) #240](https://github.com/pganalyze/libpg_query/pull/240)

Thanks @lfittl!

@gregnr Great, thanks - merged both!

Did you need tagged patch releases for your use case, or is having them in the respective branches sufficient?

@gregnr
Copy link
Contributor Author

gregnr commented Feb 20, 2024

Thanks @lfittl!

A tagged release for v13 and v14 would be amazing if possible. Otherwise I can always reference a commit instead.

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