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

Add an option to specify the output directory #104

Merged
merged 8 commits into from
Jun 21, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions src/cli_driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ struct ProcessingSession {
/// This is the virtual "CWD" that our filesystem accesses use. It is the
/// dirname of `primary_input_path`, or an empty path (i.e., corresponding
/// to the CWD if `primary_input_path` is None.
fs_root: PathBuf,
#[allow(unused)] fs_root: PathBuf,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is #[allow(unused)] necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this in because now it is unused. But maybe it will be needed later?
Else there would be a warning while compiling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see, I misunderstood, I thought the fs_root below is this one. I think that it is better to remove it. It's easy to add it back if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will do this


/// This is the name of the format file to use. TeX has to open it by name
/// internally, so it has to be String compatible.
Expand All @@ -382,6 +382,10 @@ struct ProcessingSession {
/// engine doesn't know about this path at all.
makefile_output_path: Option<PathBuf>,

/// This is the path, the processed file will be saved at. It defaults
/// to the path of `primary_input_path` or `.` if STDIN is used.
output_path: PathBuf,

pass: PassSetting,
output_format: OutputFormat,
tex_rerun_specification: Option<usize>,
Expand Down Expand Up @@ -426,6 +430,7 @@ impl ProcessingSession {

let mut io_builder = CliIoBuilder::default();
let primary_input_path;
let output_path;
let fs_root;
let tex_input_stem;

Expand Down Expand Up @@ -457,6 +462,22 @@ impl ProcessingSession {
}
}

if let Some(dir) = args.value_of("outdir") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use value_of_os to get an OsString from the argument.


Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this blank line.

output_path = {
let tmp = PathBuf::from(dir);
match tmp.extension() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this code should special-case the circumstance where the argument path has an extension. It's not common but sometimes directory names have extensions like files, and in those cases you'll get surprising behavior because the program will alter the path that you pass it.

Some(_) => {
tt_warning!(status, "file extension in `-o/--outdir`; only the directory will be used");
tmp.parent().unwrap().to_path_buf()
}
None => tmp,
}};
if !output_path.exists() { return Err(ErrorKind::Msg(format!("output directory \"{}\" does not exist", output_path.display())).into());}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't condense the bodies of if statements to single lines. That is, please put the return statement and closing bracket on their own lines.

} else {
output_path = fs_root.clone().to_path_buf()
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a double clone? Just clone() seems enough.

Copy link
Contributor Author

@h4llow3En h4llow3En Jun 17, 2017

Choose a reason for hiding this comment

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

Just clone() returns a &Path which is mismatched with PathBuf. Even if it should be equal because *PathBuf is a Path

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, fs_root is Path. In that case, fs_root.to_path_buf() (or fs_root.to_owned()) should do.

}

let mut aux_path = Path::new(tex_input_stem).to_owned();
aux_path.set_extension("aux");
let mut xdv_path = aux_path.clone();
Expand Down Expand Up @@ -501,6 +522,7 @@ impl ProcessingSession {
tex_pdf_path: pdf_path.into_os_string(),
output_format: output_format,
makefile_output_path: makefile_output_path,
output_path: output_path,
tex_rerun_specification: reruns,
keep_intermediates: args.is_present("keep_intermediates"),
keep_logs: args.is_present("keep_logs"),
Expand Down Expand Up @@ -692,8 +714,8 @@ impl ProcessingSession {
continue;
}

let mut real_path = self.fs_root.clone();
real_path.push(name);
let real_path = self.output_path.join(name);


status.note_highlighted("Writing ", &real_path.to_string_lossy(), &format!(" ({} bytes)", contents.len()));

Expand Down Expand Up @@ -1052,6 +1074,11 @@ fn main() {
.help("How much chatter to print when running.")
.possible_values(&["default", "minimal"])
.default_value("default"))
.arg(Arg::with_name("outdir")
.long("outdir")
.short("o")
.value_name("OUTDIR")
.help("The output directory. Default: Same like INPUT."))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please format the default description as it appears in the other arguments; something like

The directory in which to place output files. [default: the directory containing INPUT]

.arg(Arg::with_name("INPUT")
.help("The file to process.")
.required(true)
Expand Down
20 changes: 20 additions & 0 deletions tests/executable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,26 @@ fn test_space() {
success_or_panic(output);
}

#[test]
fn test_outdir() {
let tempdir = setup_and_copy_files(&["subdirectory/content/1.tex"]);

let output = run_tectonic(tempdir.path(),
&["--format=plain.fmt.gz", "subdirectory/content/1.tex", "--outdir=subdirectory"]);
success_or_panic(output);
check_file(&tempdir, "subdirectory/1.pdf");
}

#[test]
#[should_panic]
fn test_bad_outdir() {
let tempdir = setup_and_copy_files(&["subdirectory/content/1.tex"]);

let output = run_tectonic(tempdir.path(),
&["--format=plain.fmt.gz", "subdirectory/content/1.tex", "--outdir=subdirectory/non_existent"]);
success_or_panic(output);
}

#[test]
fn test_keep_logs_on_error() {
// No input files here, but output files are created.
Expand Down