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

Normalize paths during compilation #1702

Merged
merged 4 commits into from
Apr 2, 2024
Merged

Conversation

Ekrekr
Copy link
Contributor

@Ekrekr Ekrekr commented Mar 28, 2024

Because Node isn't used in Core, Node's path computation can't be used to do this directly. To avoid redefining path resolution in yet another place, this seems like the best option.

@Ekrekr Ekrekr requested a review from BenBirt March 28, 2024 15:16
Copy link
Collaborator

@BenBirt BenBirt left a comment

Choose a reason for hiding this comment

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

I'm not sure that having semi-support for absolute paths is a good idea (I'd probably just normalize relative paths)... but up to you to decide.

core/utils.ts Outdated

export function resolveActionsConfigFilename(configFilename: string, configPath: string) {
// This path computation supports "absolute" paths in the context of a dataform project.
if (configFilename[0] === "/") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we sure it is a good idea to start supporting absolute (ish!) paths?

I'm a bit concerned about the lack of consistency (maybe resulting in actual bugs / conflicts) with what we output, i.e. none of our output filenames contain leading slashes.

I think I'd be more comfortable doing this if we were more consistent about that. without it, it's definitely a bit worrying.

@Ekrekr
Copy link
Contributor Author

Ekrekr commented Mar 28, 2024

I'm not sure that having semi-support for absolute paths is a good idea (I'd probably just normalize relative paths)... but up to you to decide.

AFAICT special logic for absolute paths is needed even if we do normalization (so this is just a subset of supported paths):

We currently resolve filename: contents.sqlx without the leading forward slash to the directory of where the actions.yaml is - If you normalize and resolve filename: /contents.sqlx, using the path of actions.yaml as the root, it gives the exact same result.

What I think makes sense is for the root to be the root "/" of the project, and the effective current working directory to be current location of the actions.yaml file. But normalization and resolution by itself doesn't do that, because we still normalize and then resolve based on the location of the actions.yaml - meaning that "/" still needs a special case anyway.

@BenBirt
Copy link
Collaborator

BenBirt commented Mar 28, 2024

We currently resolve filename: contents.sqlx without the leading forward slash to the directory of where the actions.yaml is - If you normalize and resolve filename: /contents.sqlx, using the path of actions.yaml as the root, it gives the exact same result.

What I think makes sense is for the root to be the root "/" of the project, and the effective current working directory to be current location of the actions.yaml file. But normalization and resolution by itself doesn't do that, because we still normalize and then resolve based on the location of the actions.yaml - meaning that "/" still needs a special case anyway.

I don't follow. I'm not commenting on "normalize and resolve filename: /contents.sqlx, using the path of actions.yaml" - I don't think that's the right way to do it (I'm a bit confused about why you are mentioning it?). I'm saying that I think it would probably be safer just to make filename: ../contents.sql work, rather than /contents.sql, because of the mismatch between input and output.

@Ekrekr
Copy link
Contributor Author

Ekrekr commented Mar 28, 2024

We currently resolve filename: contents.sqlx without the leading forward slash to the directory of where the actions.yaml is - If you normalize and resolve filename: /contents.sqlx, using the path of actions.yaml as the root, it gives the exact same result.
What I think makes sense is for the root to be the root "/" of the project, and the effective current working directory to be current location of the actions.yaml file. But normalization and resolution by itself doesn't do that, because we still normalize and then resolve based on the location of the actions.yaml - meaning that "/" still needs a special case anyway.

I don't follow. I'm not commenting on "normalize and resolve filename: /contents.sqlx, using the path of actions.yaml" - I don't think that's the right way to do it (I'm a bit confused about why you are mentioning it?). I'm saying that I think it would probably be safer just to make filename: ../contents.sql work, rather than /contents.sql, because of the mismatch between input and output.

Oh right! I want to support both though - or at least throw an error if an absolute path is used and we don't support it.

What mismatch between input and output do you mean? The outputted filenames in the compiled graph are always from the base of the project (though we don't place a root slash in it), so filename: /contents.sqlx -> contents.sqlx does make sense IMO.

@BenBirt
Copy link
Collaborator

BenBirt commented Mar 28, 2024

so filename: /contents.sqlx -> contents.sqlx does make sense IMO.

Right, this is the part that I think is confusing/may lead to trouble. In input, we're suggesting that the project root is really the filesystem root - fine, makes sense. But in output, we're not. That inconsistency seems risky to me, I hope that it will not lead to issues.

@Ekrekr
Copy link
Contributor Author

Ekrekr commented Mar 28, 2024

so filename: /contents.sqlx -> contents.sqlx does make sense IMO.

Right, this is the part that I think is confusing/may lead to trouble. In input, we're suggesting that the project root is really the filesystem root - fine, makes sense. But in output, we're not. That inconsistency seems risky to me, I hope that it will not lead to issues.

That's fair, I think I agree then. So the steps would be:

  • Don't submit this, instead do normalization.
  • In a subsequent update:
    • Ensure that all compiled graph consumers are currently happy with either no leading slash, or a leading slash, being the root of the dataform project, not the root of the consumers directory system.
    • Update compiled graph outputs to include leading forward slashes.
    • Implement this in a simpler way, as resolve will actually work correctly.

@Ekrekr Ekrekr changed the title Support absolute path bases Normalize paths during compilation Apr 2, 2024
@Ekrekr
Copy link
Contributor Author

Ekrekr commented Apr 2, 2024

Have updated this PR to do the normalization instead.

@Ekrekr Ekrekr merged commit 8f288b8 into main Apr 2, 2024
4 checks passed
@Ekrekr Ekrekr deleted the support-absolute-path-bases branch April 2, 2024 10:06
moker-spaghetti pushed a commit to moker-spaghetti/dataform that referenced this pull request May 26, 2024
…-path-bases

Normalize paths during compilation
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