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

use #fileID for Swift 5.3+ #187

Merged
merged 2 commits into from
Apr 6, 2021
Merged

use #fileID for Swift 5.3+ #187

merged 2 commits into from
Apr 6, 2021

Conversation

weissi
Copy link
Member

@weissi weissi commented Mar 17, 2021

Motivation:

Right now, we use the full path (#file) for the logging as well as for
parsing out the module name (which goes into source by default).

This has two problems:

  1. The file names contain the build server's full path, ie. aren't
    portable across different compiles (and are very long).
  2. The module name parsing fails if we log from a file that's not in a
    directory named just like the module.

Modification:

Switch #file for #fileID. This changes a bunch of things:

  • source is always the module name by default (great!)
  • file changes from /the/full/path/on/disk to ModuleName/FileBaseName.swift (I think better, but it's a change)

Result:

source is more correct, file will be different.

@weissi
Copy link
Member Author

weissi commented Mar 17, 2021

@ktoso / @tomerd this is mostly an RFC

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM, it's a change in logged info but I don't think that's something we need to major bump for. For what it's worth, it was kind of sometimes broken before as you explain... 👍

@weissi
Copy link
Member Author

weissi commented Mar 17, 2021

@ktoso I agree that we don't need a major. In theory it could break some backends (which parse the path) but I don't think we really guaranteed that it's the full file path...

@weissi weissi requested a review from tomerd March 17, 2021 14:59
@weissi
Copy link
Member Author

weissi commented Apr 6, 2021

@swift-server-bot test this please

@weissi weissi force-pushed the jw-file-id branch 3 times, most recently from 43d3599 to f5d54e4 Compare April 6, 2021 13:10
Motivation:

Right now, we use the full path (`#file`) for the logging as well as for
parsing out the module name (which goes into `source` by default).

This has two problems:

1. The file names contain the build server's full path, ie. aren't
   portable across different compiles (and are very long).
2. The module name parsing fails if we log from a file that's not in a
   directory named just like the module.

Modification:

Switch `#file` for `#fileID`. This changes a bunch of things:

- `source` is always the module name by default (great!)
- `file` changes from `/the/full/path/on/disk` to `ModuleName/FileBaseName.swift` (I think better, but it's a change)

Result:

`source` is more correct, `file` will be different.
@weissi weissi merged commit 3577a99 into apple:main Apr 6, 2021
@weissi weissi deleted the jw-file-id branch April 6, 2021 13:28
@tomerd
Copy link
Member

tomerd commented Apr 6, 2021

late LGTM

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