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

Accept extensions other than .cpp in nvq++ #2514

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bebora
Copy link
Contributor

@bebora bebora commented Jan 16, 2025

Problem description

I took a look at nvq++ after reading #2512 and found the culprit:

file=$(basename -s .cc -s .cpp $i)

nvq++ seems to be written to already allow .cc files. However, basename only honors the latest -s option when multiple ones are provided:

$ basename -s .py abc.py
abc
$ basename -s .py -s .txt abc.py
abc.py
$ basename -s .txt -s .py abc.py
abc

Assume to be compiling a file named ghz.cc; during the compilation steps the following command would be run by nvq++ (simplified for clarity):

cudaq-quake --emit-llvm-file ghz.cc -o ghz.cc.qke

This command will create two files: ghz.cc.qke and ghz.ll. Note that the LLVM IR file does not have the ".cc" part.
The compilation will fail due to the unexpected filename and that specific .ll file will be kept as a leftover.

Proposed solution

basename can still be used to get a "stem.suffix" string. The suffix can then be removed using Bash Parameter Substitution.
I also added the support for .cxx and .c++ as these are common enough C++ source file extensions.

Copy link

copy-pr-bot bot commented Jan 16, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@@ -746,7 +746,8 @@ if ${SHOW_VERSION} && [ -z "$SRCS" ] && [ -z "$OBJS" ]; then
fi

for i in ${SRCS}; do
file=$(basename -s .cc -s .cpp $i)
file_with_suffix=$(basename $i)
file=${file_with_suffix%%.*}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is closer, but will fail if the source file has more than one . in the name. (It removes everything after the first ..)

Maybe let's try writing a simple function to do this instead. Something like this ought to catch the 4 cases precisely.

function remove_suffix {
  case $1 in
  *.cpp) rv=`basename -s .cpp $i` ;;
  ...
  esac
  $rv
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch.
The following line should only remove the part after the last .:

file=${file_with_suffix%.*}

Would you prefer this little modification or the custom function approach?

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