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

feat: Add command -cwd (change working directory) #1024

Merged
merged 4 commits into from
Jun 22, 2024

Conversation

helmesjo
Copy link
Contributor

@helmesjo helmesjo commented Mar 17, 2024

Makes it possible to have cppfront output generated files into a separate directory (unless -output is specified).

Before:

$ ls ..
../hello.cpp2
$ cppfront ../hello.cpp2
$ ls ..
../hello.cpp2
../hello.cpp

After (when using -cwd):

$ ls
./hello.cpp2
subdir/
$ cppfront -cwd ./subdir ./hello.cpp
$ tree
.
├── hello.cpp2
└── subdir
    └── hello.cpp

Side note:

Arguably the output path should always be the current working directory with option to retain relative sub-directory hierarchy, eg. when passing multiple files (this is how most tools operate), and not be derived from input files. Though I assumed that would break tests (and require more special handling/options).

So running cd build && cppfront [options] $(find . -name "*.h2" -o -name "*.cpp2") with this structure:

$ tree
├── one.cpp2
├── one.h2
└── subdir1
    ├── subdir2
    │   ├── three.cpp2
    │   └── three.h2
    ├── two.cpp2
    └── two.h2
$ mkdir build && cd build && cppfront [options] $(find . -name "*.h2" -o -name "*.cpp2")

could yield this result:

$ tree
├── build
│   ├── one.cpp
│   ├── one.h
│   └── subdir1
│       ├── subdir2
│       │   ├── three.cpp
│       │   └── three.h
│       ├── two.cpp
│       └── two.h
├── one.cpp2
├── one.h2
└── subdir1
    ├── subdir2
    │   ├── three.cpp2
    │   └── three.h2
    ├── two.cpp2
    └── two.h2

@hsutter
Copy link
Owner

hsutter commented Mar 20, 2024

Thanks for your pull request! It looks like this may be your first contribution to cppfront. I've emailed you the Contributor License Agreement (CLA), and once it's signed I can look at your pull request. Thanks again for your contribution.

@helmesjo
Copy link
Contributor Author

Thanks for your pull request! It looks like this may be your first contribution to cppfront. I've emailed you the Contributor License Agreement (CLA), and once it's signed I can look at your pull request. Thanks again for your contribution.

Sure thing! Do you mind re-sending it though? My email got a bit mixed up so you sent it to the wrong one.

@hsutter
Copy link
Owner

hsutter commented Mar 21, 2024

Thanks! This is new to me as I'm only familiar with changing the working directory in shell scripts before invoking the compiler, so I have some questions...

Can you point me to a similar existing flag on another compiler I could take a look at to understand the prior art better? Then I could look at examples and what other compilers do for this feature.

A question about this example:

After (when using -cwd):

$ ls
./hello.cpp2
subdir/
$ cppfront -cwd ./subdir ./hello.cpp
$ tree
.
├── hello.cpp2
└── subdir
    └── hello.cpp

I'm slightly confused... cppfront -cwd ./subdir ./hello.cpp doesn't work for me in this branch.

Is that intended to be cppfront -cwd ./subdir ../hello.cpp2 ? I do get the above result with that.

Today I can write cppfront -o subdir/hello.cpp hello.cpp2... that seems to have the same effect, if I understand right? I think what you're saying is that the -cwd approach lets you avoid repeating part of the filename?

mkdir build && cd build && cppfront [options] $(find . -name "*.h2" -o -name "*.cpp2")

I tried variations of this but wasn't able to get this to work in bash?

Thanks for your patience with all the questions! It's just because it's unfamiliar to me so I want to be sure I understand. Thanks again for the suggestion!

@helmesjo
Copy link
Contributor Author

helmesjo commented Mar 21, 2024

Thanks! This is new to me as I'm only familiar with changing the working directory in shell scripts before invoking the compiler, so I have some questions...

Can you point me to a similar existing flag on another compiler I could take a look at to understand the prior art better? Then I could look at examples and what other compilers do for this feature.

Agree (and I do the same), the crux here though is that cppfront will output the file next to the .h2|.cpp2 input file being processed (and ignore my current working directory).
And to elaborate on my situation I'm facing, I've packaged cppfront (although not completely finished but all regression-tests pass) for build2 during the "config phase" (in the make world) I first extract which files cppfront will generate and feed this to build2. And preferably I'd like to pass all files in one go (I'll probably try to add a PR to add a "list files that would be generated" so you don't actually have to generate them in the first pass, only during build). For things to work out afterwards when building the generated cpp1 files (with #includes matching the expected structure, eg. #include <subdir/header.hpp>) the output would need to be (or at least it would make sense to be) in the same hierarchy (but potentially in another build directory, out-of-source).

But (as evident from my many edits to the description for this PR), as I was adding it I got a feeling this middle-ground implementation of touching as little as possible but cover my specific use-case probably is redundant and I'll look into changing it to be, as you said, just cd before invoking cppfront while still deadling with any number of input files (which AFAICS is not the case currently, but instead cppfront will output each cpp1 file next to the cpp2 one). Again, assuming I'm passing multiple cpp2 files at once and thus can't use the -output option:

-output filename, -o filename
Output to 'filename' (can be 'stdout'). If not set, the default output filename for is the same as the input filename without the 2 > (e.g., compiling hello.cpp2 by default writes its output to hello.cpp, and header.h2 to header.h).

Long story short, let's keep this as "Work In Progress" and I'll ping when I have cooked something up. :)

@hsutter
Copy link
Owner

hsutter commented Mar 21, 2024

Actually, I think you've identified a real issue that should be easy to fix: Compilers generally put their output in the current directory. Cppfront should, too.

When I run:

$ cppfront ../demo.cpp2
$ g++ ../demo.cpp

the first line puts the output file in the parent directory, the second in the current directory.

So I think cppfront should just put files in the current directly by default, like all compilers do.

@JohelEGP / @MaxSagebaum / everyone: Would making that change destabilize any of you, such as this Godbolt environment ?

@helmesjo
Copy link
Contributor Author

Actually, I think you've identified a real issue that should be easy to fix: Compilers generally put their output in the current directory. Cppfront should, too.

When I run:

$ cppfront ../demo.cpp2
$ g++ ../demo.cpp

the first line puts the output file in the parent directory, the second in the current directory.

So I think cppfront should just put files in the current directly by default, like all compilers do.

@JohelEGP / @MaxSagebaum / everyone: Would making that change destabilize any of you, such as this Godbolt environment ?

Yep, that was my plan as well. Especially useful when you want cppfront to handle all naming of files (but still put them relative to cwd).

@MaxSagebaum
Copy link
Contributor

@JohelEGP / @MaxSagebaum / everyone: Would making that change destabilize any of you, such as this Godbolt environment ?

I usually prefer the -o option. But, I see the merit to align it to the standard behavior. The cwd option seems a good idea.

@hsutter
Copy link
Owner

hsutter commented May 3, 2024

Picking this up again after the April madness:

I think we ended up agreeing that ideally cppfront would put files in the current directory by default. Is that right?

My main question before doing that is whether it would destabilize the Godbolt environment linked above, or similar uses?

Thanks again for pointing out this area needs improvement, however we end up improving it!

@helmesjo
Copy link
Contributor Author

helmesjo commented May 5, 2024

Picking this up again after the April madness:

I think we ended up agreeing that ideally cppfront would put files in the current directory by default. Is that right?

My main question before doing that is whether it would destabilize the Godbolt environment linked above, or similar uses?

Thanks again for pointing out this area needs improvement, however we end up improving it!

My suggestion was to output to the current working directory, though mirror the input tree for the output(s). If relative includes are used that approach wouldn't interfere (maybe this was what you meant?).

@jarzec
Copy link
Contributor

jarzec commented May 12, 2024

For what it's worth I would suggest to avoid this kind of situation and go for what compilers already do as much as possible.
Sticking to what GCC and Clang do would be along the lines of one of the funding ideas behind cppfront: making tooling simpler (or in this case not more complicated). Creating project files, like CMakeLists.txt, will me much easier without special cases for cppfront.

@helmesjo
Copy link
Contributor Author

helmesjo commented Jun 13, 2024

For what it's worth I would suggest to avoid this kind of situation and go for what compilers already do as much as possible. Sticking to what GCC and Clang do would be along the lines of one of the funding ideas behind cppfront: making tooling simpler (or in this case not more complicated). Creating project files, like CMakeLists.txt, will me much easier without special cases for cppfront.

I agree, though just to highlight the difference here:
cppfront generates only an intermediate (source) step which then needs to be compiled, which in turn requires the previous structure to work properly (otherwise only simple "hello.h" kind of includes are supported and not eg. "libhello/utils/hello.h" or my preferred <libhello/utils/hello.h>, unless one manually moves files around afterwards). Compare this to generated object files that don't care about directory hierarchy.
Given this (and that we want to stay similar to GCC which disregards the input file(s) hierarchy & just outputs to the cwd), is an option to add a flag -preserve-hierarchy (or similar) to achieve this?

I imagine that a useful and easy to grasp approach could be that:

  1. -preserve-hierarchy: Input file(s) full path is preserved, and cppfront only changes extension (drops the 2).
  2. -output-dir: Specifies the root-dir for all generated output. Default is cwd (if 1. is set and the input file path(s) are absolute paths then this flag naturally has no effect).

This way the behavior should remain the same (all tests are unaffected).
But I can still (opt-in) achieve this:

// hello.h2
#include <subdir/world.h>
// ..
$ tree
.
├── hello.cpp2
├── hello.h2
└── subdir
    └── world.h2
$ cppfront -preserve-hierarchy -output-dir /tmp/test ./hello.cpp2 ./hello.h2 ./subdir/world.h2
$ tree /tmp/test
/tmp/test/
├── hello.cpp
├── hello.h
└── subdir
    └── world.h
# compile ...

Basically: The include written in hello.h2 would work as expected (IMO). That is, I wouldn't have to spend cognitive load on figuring out intrinsics about cppfront, the result "just works".

I'll experiment a bit but please chip in with feedback.

@hsutter
I noticed that (AFAICS) there are no tests exorcising includes. I guess that would be a good addition regardless of this PR?

Copy link
Owner

@hsutter hsutter left a comment

Choose a reason for hiding this comment

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

Thanks for waiting. I think with the suggested change we get both things -- -cwd plus changing the default to put the Cpp1 output in the current directory. Final sanity check: Does that plan seem sensible?

source/to_cpp1.h Outdated
@@ -164,6 +164,15 @@ static cmdline_processor::register_flag cmd_cpp1_filename(
[](std::string const& name) { flag_cpp1_filename = name; }
);

static auto flag_cwd = std::filesystem::path{};;
Copy link
Owner

Choose a reason for hiding this comment

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

Double ;;

source/to_cpp1.h Outdated
if (!flag_cpp1_filename.empty()) { // use override if present
cpp1_filename = flag_cpp1_filename;
}
else if (!flag_cwd.empty()) { // strip leading path if cwd was explicitly set
Copy link
Owner

Choose a reason for hiding this comment

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

If we make this like be just else {, then I think the semantics would be that you get your desired behavior if -cwd is specified, and we get the desired behavior change of putting the output in the current directory, correct?

…ath is stripped

In addition to enabling `-cwd` behavior, this will change the default to put files in the current directory even if a pathname is used to refer to a file in another directory (this default behavior will be overridden by `-o` or `-cwd` if specified)
@hsutter hsutter merged commit 67f5e0e into hsutter:main Jun 22, 2024
26 checks passed
@hsutter
Copy link
Owner

hsutter commented Jun 22, 2024

Thanks! @helmesjo when you get a chance would you please check that the commit I pushed onto this PR before merging still keeps the behavior that you wanted?

@helmesjo
Copy link
Contributor Author

@hsutter Will do, and sorry for the delay!

@hsutter
Copy link
Owner

hsutter commented Jun 25, 2024

No worries, you've been waiting on me for a while! So have others, gradually working my way through the backlog...

MarekKnapek pushed a commit to MarekKnapek/cppfront that referenced this pull request Jul 4, 2024
* cli: Add command '-cwd' (change working directory).

* Tweak to strip leading path always, so that the `.cpp2` source file path is stripped

In addition to enabling `-cwd` behavior, this will change the default to put files in the current directory even if a pathname is used to refer to a file in another directory (this default behavior will be overridden by `-o` or `-cwd` if specified)

* Remove stray `;;` for clean builds

---------

Co-authored-by: Herb Sutter <herb.sutter@gmail.com>
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.

4 participants