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.

Sign up for GitHub

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

Jump to bottom

Moves unnecessary deps out of critical build path #645

Merged
merged 12 commits into from
Aug 26, 2022
Merged

Moves unnecessary deps out of critical build path #645

merged 12 commits into from
Aug 26, 2022

Conversation

BradyBonnette
Copy link
Contributor

These are changes that coincide with #597

Mostly these changes are spent trying to pull out as much stuff out of pgx-utils where applicable, but there could be more to it as well.

@BradyBonnette
Copy link
Contributor Author

Also note that this branch is experimental for now, and the major reason for creating a pull request at this moment is to get CI to build it and see what results from the build. Depending on how that goes and the feedback given, this could very well end up in develop.

@workingjubilee
Copy link
Member

So I noticed you inlined single-consumer things into their consumers. pgx-pg-sys-stub is such a single-consumer entity. Would it be better to lock that down?

workingjubilee
workingjubilee reviewed Aug 23, 2022
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Thank you for taking a look at this!

I have some thoughts about a few more moves in this game of shufflepuck, but stripping pgx-utils down to almost the bare essentials is very nice to see.

pgx-paths/src/lib.rs Outdated Show resolved Hide resolved
pgx-paths/src/lib.rs Outdated Show resolved Hide resolved
pgx-paths/src/lib.rs Outdated Show resolved Hide resolved
pgx-utils/src/lib.rs Show resolved Hide resolved
@BradyBonnette
Some refactorings:
70db936 
* Removed pgx-paths (yes!)
* Inlined versioned_so_name
* Added "paths" methods into pgx-pg-config and exported appropriately
workingjubilee
workingjubilee reviewed Aug 23, 2022
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

The pgx-pg-sys-stub crate could probably still be moved in as a submodule in cargo-pgx.

cargo-pgx/src/command/install.rs Outdated Show resolved Hide resolved
pgx-utils/src/sql_entity_graph/pgx_sql.rs Outdated Show resolved Hide resolved
@BradyBonnette
Copy link
Contributor Author

@workingjubilee I added a couple new commits based on earlier discussions. Let me know what you think!

workingjubilee
workingjubilee approved these changes Aug 25, 2022
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Excellent!

@BradyBonnette
Copy link
Contributor Author

@workingjubilee One last closing thought before this gets merged in.

We had talked about possibly renaming pgx-utils into something more meaningful, such as your suggested pgx-sqlgen since the only thing that's left in pgx-utils now is SQL generation stuff.

Do you feel that we should still do that? I bring it up since renaming it would (more than likely) break some things for any other project that relies on PGX, but since the next release is going to be breaking anyways it might be the best opportunity to do it. If it's something we should consider, then the best course of action might be to create a separate issue just for the rename, and maybe do it right before release so we're not stomping on all of the other incoming changes from the other contributors. The things I refactored out of pgx-utils in this PR were fairly benign.

@workingjubilee
Copy link
Member

workingjubilee commented Aug 25, 2022 •
edited
Loading

Functionally, pgx-utils is really not meant to be used by external consumers to pgx, despite the fact we must publish it in order to make pgx work. However, I agree that we should leave things as-is at the current moment and have a moment to paint a bikeshed.

@BradyBonnette BradyBonnette mentioned this pull request Aug 26, 2022
Closed
@BradyBonnette
Copy link
Contributor Author

Ok, that's fair. I went ahead and made a new issue so that maybe we won't forget and perhaps visit in the future: #655

@BradyBonnette BradyBonnette merged commit ab650e6 into develop Aug 26, 2022
@BradyBonnette BradyBonnette mentioned this pull request Aug 31, 2022
Closed
BradyBonnette added a commit that referenced this pull request Sep 9, 2022
@BradyBonnette
Updates versions script because of #645
5a0a0f7
@BradyBonnette BradyBonnette mentioned this pull request Sep 9, 2022
Merged
workingjubilee pushed a commit that referenced this pull request Sep 9, 2022
@BradyBonnette
Updates versions script because of #645 (#679)
a0afc3d
@sumerman sumerman mentioned this pull request Feb 15, 2023
Merged
@BradyBonnette BradyBonnette deleted the brady/experiment-with-issue-597-attempt-three branch June 20, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BradyBonnette @workingjubilee