Skip to content

Avoiding Odin's xml config file to be considered as arch file #1758

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

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

sdamghan
Copy link
Member

@sdamghan sdamghan commented Jun 2, 2021

Signed-off-by: Seyed Alireza Damghani [email protected]

Description

the following warnings are showing while dev/upgrade_vtr_archs.sh is called. Odin's XMLs are to read the configuration of each run. There is no need to go through the Odin directory looking for XML files.

Related Issue

Motivation and Context

removing warning generated by upgrade_vtr_arch.sh

How Has This Been Tested?

Types of changes

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@github-actions github-actions bot added the Odin Odin II Logic Synthesis Tool: Unsorted item label Jun 3, 2021
@sdamghan sdamghan requested a review from vaughnbetz June 4, 2021 03:11
@sdamghan sdamghan changed the title [FLOW]: Avoiding Odin's xml config file to be considered as arch file Avoiding Odin's xml config file to be considered as arch file Jun 4, 2021
@vaughnbetz
Copy link
Contributor

vaughnbetz commented Jun 7, 2021

@sdamghan : I think you should add a comment in some appropriate place about why you use the .oxml extension though (so the flow scripts don't think this is an architecture file).

Is there any downside to renaming these files .oxml? E.g. can you open them with an xml viewer? Not a major concern for me if it doesn't bother you, but thought I'd ask in case there is an alternative way to get this warning to go away (e.g. make upgrade_vtr_archs.sh a bit smarter, having it avoid files that have _odin.xml in their name, or make the flow script calling it a bit smarter, etc.

@sdamghan
Copy link
Member Author

sdamghan commented Jun 8, 2021

@sdamghan : I think you should add a comment in some appropriate place about why you use the .oxml extension though (so the flow scripts don't think this is an architecture file).

Is there any downside to renaming these files .oxml? E.g. can you open them with an xml viewer? Not a major concern for me if it doesn't bother you, but thought I'd ask in case there is an alternative way to get this warning to go away (e.g. make upgrade_vtr_archs.sh a bit smarter, having it avoid files that have _odin.xml in their name, or make the flow script calling it a bit smarter, etc.

@vaughnbetz Odin's configuration file is actually an XML file. To remove the warnings related to Odin in upgrade_vtr_archs.sh, there are two approaches. First, we can specify not to read any XML files in the Odin directory (this is actually how we can make the flow script calling smarter). However, I think it is not a good approach since deciding about a specific behaviour using file names, not file extensions, may be confusing for users and future developers. Second, we can work on the extension of Odin's configuration file to avoid misinterpretation by the upgrade_vtr_archs.sh script. The extension .oxml is actually the same concept you mentioned about _odin.xml, and the point is that the configuration file is still an XML file, and it is read using an XML reader. I used OXML because it still shows this file is an XML file, in addition to making Odin's configuration files unique compared to regular architecture files. Only one point could be a downside for the OXML files: operating systems cannot recognize it as an XML file that I think is not a major concern. Please let me know your thoughts; once we agree on a solution, I will modify the documentation as well.

@vaughnbetz
Copy link
Contributor

I actually think it is more clear if we keep the standard extension (.xml) so the format is obvious. That's usually what a filename extension means -- if it is a standard format, the extension says what it is. That lets OS'es figure out how to open it etc.

If we can just change upgrade_vtr_archs.sh to skip the odin-II files (since it doesn't need to upgrade them or know how to do it) I think that's best. Is simply removing OdinII from the directory list at https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/dev/upgrade_vtr_archs.sh doable?

If that's not doable, using ODIN-II config files that are something like _odin.xml would let us remove them with a filename matching rule in the upgrade_vtr_archs.sh script, which I think is also OK.

@github-actions github-actions bot removed the Odin Odin II Logic Synthesis Tool: Unsorted item label Jun 8, 2021
@vaughnbetz
Copy link
Contributor

Thanks. This is a nice simple fix!
The CI failures are unrelated (due to CI refactoring).

@vaughnbetz vaughnbetz merged commit bbe0b3d into verilog-to-routing:master Jun 9, 2021
@sdamghan sdamghan deleted the arch_warnings branch November 19, 2021 18:48
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