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

Map source absolute paths to OUT_DIR as relative. #684

Merged
merged 3 commits into from
Nov 24, 2022

Conversation

dot-asm
Copy link
Contributor

@dot-asm dot-asm commented Jun 1, 2022

If a source file was specified by absolute path, then the corresponding
object file was placed into OUT_DIR. This posed a problem if multiple
files with the same base name were specified by absolute paths.

Fixes #683.

@de-vri-es
Copy link

de-vri-es commented Jul 21, 2022

This method can still cause multiple distinct paths to be mapped to the same object file.

For example, all these source files will be mapped to the same object file:

foo/bar.c          -> OUT_DIR/foo/bar.o
/home/me/foo/bar.c -> OUT_DIR/foo/bar.o
/home/foo/bar.c    -> OUT_DIR/foo/bar.o

A different option could be to include a hash of the source path in the object name. That avoids clashes without producing very long file paths.

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 22, 2022

Well, arguably suggested scenario is unlikely to be a common case, but I've added unique prefix based on absolute path for maintainers to consider :-)

@de-vri-es
Copy link

de-vri-es commented Jul 25, 2022

I'd remove the path grafting entirely now. I'd even consider doing the same for relative paths and dump all object files in one directory. Although I guess it's nice to mimic the directory structure for relative paths.

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 25, 2022

I'd remove the path grafting entirely now.

Even for relative paths? The original idea was to be as conservative as possible. I mean not changing the way it currently works in the most common case of relative paths. As for absolute paths, yeah, I've thought of "flattening" and reckoned that keeping traces of the directory structure could be helpful for trouble-shooting purposes. But sure, it's definitely a viable option. For both relative and absolute paths. Nobody traverses the object files' destination directories anyway...

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 25, 2022

I've thought of "flattening" and reckoned that keeping traces of the directory structure could be helpful for trouble-shooting purposes.

On the other hand, one can totally say that "flattening" would actually be more a conservative approach, because that's how the absolute paths are treated now. Fair enough, I'll remove the grafting for absolute paths and let maintainers choose [or express their preferences]...

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 26, 2022

Note for maintainers. There are three suggestions to consider, one per commit. First one attempts to graft subdirectories of absolute source files' paths to output directory. The second one prefixes object files with source directories' hashes. And the third one removes grafting. There is also suggestion to use hashes as prefixes even with relative paths.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This seems fine -- I prefer this approach to flattening.

One concern might be if the hashing in the filename defeats the ability of tools like sccache to cache, but I think they should only care about input filenames.

@dot-asm dot-asm force-pushed the compiling-absolute-paths branch from 1bf8899 to e151b9e Compare November 3, 2022 11:27
@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 3, 2022

This seems fine -- I prefer this approach to flattening.

Could you clarify the course of further actions for me here. I mean should I remove all commits but the fist here, or do you simply cherry-pick it yourself?

If a source file was specified by absolute path, then the corresponding
object file was placed into OUT_DIR. This posed a problem if multiple
files with the same base name were specified by absolute paths.

Fixes rust-lang#683.
@dot-asm dot-asm force-pushed the compiling-absolute-paths branch from e151b9e to 36137ae Compare November 24, 2022 00:16
@thomcc
Copy link
Member

thomcc commented Nov 24, 2022

Sorry, that wasn't about the commit structure. I just forgot to merge

@thomcc thomcc merged commit c4f414f into rust-lang:main Nov 24, 2022
roblabla added a commit to roblabla/cc-rs that referenced this pull request Jan 3, 2024
Since rust-lang#684 was merged, it is
impossible for `obj` to not start with dst - if `obj` is absolute or
contains `../` in its Path, then the path will be hashed, and the file
will still be placed under dst.

As such, this obj.starts_with(&dst) check is always true.
roblabla added a commit to roblabla/cc-rs that referenced this pull request Jan 23, 2024
Since rust-lang#684 was merged, it is
impossible for `obj` to not start with dst - if `obj` is absolute or
contains `../` in its Path, then the path will be hashed, and the file
will still be placed under dst.

As such, this obj.starts_with(&dst) check is always true.
NobodyXu pushed a commit that referenced this pull request Jan 24, 2024
* Add new compile_intermediates function.

This new function can be used to just compile the files to a bunch of .o
files.

* Pre-allocate objects vec in objects_from_files

* Remove some dead code in objects_from_file

Since #684 was merged, it is
impossible for `obj` to not start with dst - if `obj` is absolute or
contains `../` in its Path, then the path will be hashed, and the file
will still be placed under dst.

As such, this obj.starts_with(&dst) check is always true.

* Always hash the file prefix

* Fix error handling of objects_from_files

* Fix nightly warning
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.

Bug: Duplicate symbols if there multiple source files have the same name
3 participants